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

[Tycho Andersen]

Hi Eugene,

On Thu, Aug 04, 2016 at 07:01:36PM +0300, Eugene Batalov wrote:
> 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?

Sorry, I was speaking of the case I already mentioned, about trying to
use criu gc to clean up after a criu dump --leave-stopped. I think
this patchset doesn't handle that case because of the missing
cpt_unlock_tcp_connections(); but it sounds like we don't care about
that, so never mind :)

Tycho

> 
> >
> > > 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.