[CRIU] [PATCH v1 05/12] gc: implement unlocking of tcp connections

[Eugene Batalov]

2016-08-04 18:53 GMT+03:00 Tycho Andersen <tycho.andersen at canonical.com>:

> On Thu, Aug 04, 2016 at 06:42:06PM +0300, Eugene Batalov wrote:
> > 2016-08-04 18:08 GMT+03:00 Tycho Andersen <tycho.andersen at canonical.com>
> :
> >
> > > Hi Eugene,
> > >
> > > On Thu, Aug 04, 2016 at 02:49:23PM +0300, Eugene Batalov wrote:
> > > > Hi Tycho,
> > > >
> > > > > > +int gc_network_unlock(void)
> > > > > > +{
> > > > > > +     /*
> > > > > > +      * Unshared ps tree net ns is destroyed after successful
> dump.
> > > > > > +      * No need to call network_unlock_internal.
> > > > > > +      * Also don't call ACT_NET_UNLOCK script because we don't
> > > > > > +      * resume/restore ps tree - this call would break
> > > > > > +      * ACT_NET_UNLOCK semantics.
> > > > > > +      */
> > > > > > +     return rst_unlock_tcp_connections();
> > > > >
> > > > > What about cpt_unlock_tcp_connections()? IIUC this list is not
> > > > > persisted, and so if we leave around the network lock stuff, we
> would
> > > > > never turn of TCP repair mode.
> > > > >
> > > > > Tycho
> > > > >
> > > > Let's consider the moment when we start criu gc or criu restore. At
> > > > this moment dumpee ps tree doesn't exist and ps tree sockets don't
> > > > exist.
> > >
> > > Hmm. Perhaps I misunderstood then. I thought the point was to be able
> > > to use --leave-stopped (so the ps tree would still exist), and then we
> > > could at a later time run `criu gc` to unlock things again and clean
> > > this up.
> > >
> > Looks like this is the use case you need to support. Let's look at
> current
> > implementation. Does it satisfies your needs?
>
> I worked around my issue in another way, so I think we can ignore my
> needs for the purpose of `criu gc`. If we wanted to clean up after a
> dump that left the network locked, I don't think this set would do it
> completely, but it sounds like we may not care about that.
>
Could you propose an example when this patch set doesn't unlock the network?


>
> > We can kill whole ps tree with any standard signal based logic so it
> looks
> > like there is no problem here.
> > But is it possible for you to have dump of this killed ps tree. criu gc
> > needs this dump in order to do its job.
> > Also criu gc requires only subset of the dump (memory is not needed).
> >
> > For now I prefer current criu gc implementation that works with dumps
> > rather then with live ps trees.
> > In general If we want to c/r we always create dump from live ps tree but
> we
> > don't always restore ps tree from dump.
> > So dump is more basic thing that criu gc relies on. The only question is
> > about availability of such a dump for gc.
>
> Ok, but I still don't understand, what's the use case for something this?
>
I have some thoughts like "if we don't restore ps tree we leave some
garbage in the system and we should be able to remove this garbage".
Maybe Pavel can tell us about more important use case.


>
> > >
> > > > That's why we don't need to turn any tcp repair mode off - it is per
> > > socket
> > > > thing.
> > > > Also cpt_unlock_tcp_connections called from network_unlock() that is
> > > called
> > > > during criu restore
> > > > does nothing because cpt_tcp_repair_sockets list is empty. So for me
> it
> > > > looks like criu gc works
> > > > the same as criu restore but without calling of network unlock action
> > > > script.
> > >
> > > I understand; it may be worth noting somewhere in the help output or
> > > patch notes what exactly the use case is. Right now I don't undersand
> > > what it is :)
> > >
> >  Yep. I'll add more info into criu help message in the next patch set
> > version. Thank you for suggestion.
>
> OK :)
>
> Tycho
>
> > >
> > > Tycho
> > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > >  int veth_pair_add(char *in, char *out)
> > > > > >  {
> > > > > >       char *aux;
> > > > > > diff --git a/criu/sk-tcp.c b/criu/sk-tcp.c
> > > > > > index 13d175a..c4aa58f 100644
> > > > > > --- a/criu/sk-tcp.c
> > > > > > +++ b/criu/sk-tcp.c
> > > > > > @@ -713,16 +713,19 @@ void tcp_locked_conn_add(struct
> inet_sk_info
> > > *ii)
> > > > > >       ii->sk_fd = -1;
> > > > > >  }
> > > > > >
> > > > > > -void rst_unlock_tcp_connections(void)
> > > > > > +int rst_unlock_tcp_connections(void)
> > > > > >  {
> > > > > >       struct inet_sk_info *ii;
> > > > > >
> > > > > >       /* Network will be unlocked by network-unlock scripts */
> > > > > >       if (root_ns_mask & CLONE_NEWNET)
> > > > > > -             return;
> > > > > > +             return 0;
> > > > > >
> > > > > >       list_for_each_entry(ii, &rst_tcp_repair_sockets, rlist)
> > > > > > -             nf_unlock_connection_info(ii);
> > > > > > +             if (nf_unlock_connection_info(ii))
> > > > > > +                     return -1;
> > > > > > +
> > > > > > +     return 0;
> > > > > >  }
> > > > > >
> > > > > >  int check_tcp(void)
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > CRIU mailing list
> > > > > > CRIU at openvz.org
> > > > > > https://lists.openvz.org/mailman/listinfo/criu
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > > Eugene Batalov.
> > >
> >
> >
> >
> > --
> > Best regards,
> > Eugene Batalov.
>



-- 
Best regards,
Eugene Batalov.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20160804/992aca82/attachment.html>