<div dir="ltr"><div>Pavel, it turned out that criu gc doesn't solve Tycho's use case because he wants to perform continuation of live ps tree after <span style="font-size:12.8px">criu dump --leave-stopped. Implementation of this feature is very different comparing to implementation of criu gc that uses dumps as source of information about dead ps tree garbage.</span></div><div><span style="font-size:12.8px">So do we still need new criu gc command? If yes then I'll send v2 patch set with extended help message for the command.</span></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-08-04 19:26 GMT+03:00 Tycho Andersen <span dir="ltr"><<a href="mailto:tycho.andersen@canonical.com" target="_blank">tycho.andersen@canonical.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Eugene,<br>
<div><div class="h5"><br>
On Thu, Aug 04, 2016 at 07:01:36PM +0300, Eugene Batalov wrote:<br>
> 2016-08-04 18:53 GMT+03:00 Tycho Andersen <<a href="mailto:tycho.andersen@canonical.com">tycho.andersen@canonical.com</a>><wbr>:<br>
><br>
> > On Thu, Aug 04, 2016 at 06:42:06PM +0300, Eugene Batalov wrote:<br>
> > > 2016-08-04 18:08 GMT+03:00 Tycho Andersen <<a href="mailto:tycho.andersen@canonical.com">tycho.andersen@canonical.com</a>><br>
> > :<br>
> > ><br>
> > > > Hi Eugene,<br>
> > > ><br>
> > > > On Thu, Aug 04, 2016 at 02:49:23PM +0300, Eugene Batalov wrote:<br>
> > > > > Hi Tycho,<br>
> > > > ><br>
> > > > > > > +int gc_network_unlock(void)<br>
> > > > > > > +{<br>
> > > > > > > + /*<br>
> > > > > > > + * Unshared ps tree net ns is destroyed after successful<br>
> > dump.<br>
> > > > > > > + * No need to call network_unlock_internal.<br>
> > > > > > > + * Also don't call ACT_NET_UNLOCK script because we don't<br>
> > > > > > > + * resume/restore ps tree - this call would break<br>
> > > > > > > + * ACT_NET_UNLOCK semantics.<br>
> > > > > > > + */<br>
> > > > > > > + return rst_unlock_tcp_connections();<br>
> > > > > ><br>
> > > > > > What about cpt_unlock_tcp_connections()? IIUC this list is not<br>
> > > > > > persisted, and so if we leave around the network lock stuff, we<br>
> > would<br>
> > > > > > never turn of TCP repair mode.<br>
> > > > > ><br>
> > > > > > Tycho<br>
> > > > > ><br>
> > > > > Let's consider the moment when we start criu gc or criu restore. At<br>
> > > > > this moment dumpee ps tree doesn't exist and ps tree sockets don't<br>
> > > > > exist.<br>
> > > ><br>
> > > > Hmm. Perhaps I misunderstood then. I thought the point was to be able<br>
> > > > to use --leave-stopped (so the ps tree would still exist), and then we<br>
> > > > could at a later time run `criu gc` to unlock things again and clean<br>
> > > > this up.<br>
> > > ><br>
> > > Looks like this is the use case you need to support. Let's look at<br>
> > current<br>
> > > implementation. Does it satisfies your needs?<br>
> ><br>
> > I worked around my issue in another way, so I think we can ignore my<br>
> > needs for the purpose of `criu gc`. If we wanted to clean up after a<br>
> > dump that left the network locked, I don't think this set would do it<br>
> > completely, but it sounds like we may not care about that.<br>
> ><br>
> Could you propose an example when this patch set doesn't unlock the network?<br>
<br>
</div></div>Sorry, I was speaking of the case I already mentioned, about trying to<br>
use criu gc to clean up after a criu dump --leave-stopped. I think<br>
this patchset doesn't handle that case because of the missing<br>
cpt_unlock_tcp_connections(); but it sounds like we don't care about<br>
that, so never mind :)<br>
<span class="HOEnZb"><font color="#888888"><br>
Tycho<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> ><br>
> > > We can kill whole ps tree with any standard signal based logic so it<br>
> > looks<br>
> > > like there is no problem here.<br>
> > > But is it possible for you to have dump of this killed ps tree. criu gc<br>
> > > needs this dump in order to do its job.<br>
> > > Also criu gc requires only subset of the dump (memory is not needed).<br>
> > ><br>
> > > For now I prefer current criu gc implementation that works with dumps<br>
> > > rather then with live ps trees.<br>
> > > In general If we want to c/r we always create dump from live ps tree but<br>
> > we<br>
> > > don't always restore ps tree from dump.<br>
> > > So dump is more basic thing that criu gc relies on. The only question is<br>
> > > about availability of such a dump for gc.<br>
> ><br>
> > Ok, but I still don't understand, what's the use case for something this?<br>
> ><br>
> I have some thoughts like "if we don't restore ps tree we leave some<br>
> garbage in the system and we should be able to remove this garbage".<br>
> Maybe Pavel can tell us about more important use case.<br>
><br>
><br>
> ><br>
> > > ><br>
> > > > > That's why we don't need to turn any tcp repair mode off - it is per<br>
> > > > socket<br>
> > > > > thing.<br>
> > > > > Also cpt_unlock_tcp_connections called from network_unlock() that is<br>
> > > > called<br>
> > > > > during criu restore<br>
> > > > > does nothing because cpt_tcp_repair_sockets list is empty. So for me<br>
> > it<br>
> > > > > looks like criu gc works<br>
> > > > > the same as criu restore but without calling of network unlock action<br>
> > > > > script.<br>
> > > ><br>
> > > > I understand; it may be worth noting somewhere in the help output or<br>
> > > > patch notes what exactly the use case is. Right now I don't undersand<br>
> > > > what it is :)<br>
> > > ><br>
> > > Yep. I'll add more info into criu help message in the next patch set<br>
> > > version. Thank you for suggestion.<br>
> ><br>
> > OK :)<br>
> ><br>
> > Tycho<br>
> ><br>
> > > ><br>
> > > > Tycho<br>
> > > ><br>
> > > > > ><br>
> > > > > > > +}<br>
> > > > > > > +<br>
> > > > > > > int veth_pair_add(char *in, char *out)<br>
> > > > > > > {<br>
> > > > > > > char *aux;<br>
> > > > > > > diff --git a/criu/sk-tcp.c b/criu/sk-tcp.c<br>
> > > > > > > index 13d175a..c4aa58f 100644<br>
> > > > > > > --- a/criu/sk-tcp.c<br>
> > > > > > > +++ b/criu/sk-tcp.c<br>
> > > > > > > @@ -713,16 +713,19 @@ void tcp_locked_conn_add(struct<br>
> > inet_sk_info<br>
> > > > *ii)<br>
> > > > > > > ii->sk_fd = -1;<br>
> > > > > > > }<br>
> > > > > > ><br>
> > > > > > > -void rst_unlock_tcp_connections(<wbr>void)<br>
> > > > > > > +int rst_unlock_tcp_connections(<wbr>void)<br>
> > > > > > > {<br>
> > > > > > > struct inet_sk_info *ii;<br>
> > > > > > ><br>
> > > > > > > /* Network will be unlocked by network-unlock scripts */<br>
> > > > > > > if (root_ns_mask & CLONE_NEWNET)<br>
> > > > > > > - return;<br>
> > > > > > > + return 0;<br>
> > > > > > ><br>
> > > > > > > list_for_each_entry(ii, &rst_tcp_repair_sockets, rlist)<br>
> > > > > > > - nf_unlock_connection_info(ii);<br>
> > > > > > > + if (nf_unlock_connection_info(ii)<wbr>)<br>
> > > > > > > + return -1;<br>
> > > > > > > +<br>
> > > > > > > + return 0;<br>
> > > > > > > }<br>
> > > > > > ><br>
> > > > > > > int check_tcp(void)<br>
> > > > > > > --<br>
> > > > > > > 1.9.1<br>
> > > > > > ><br>
> > > > > > > ______________________________<wbr>_________________<br>
> > > > > > > CRIU mailing list<br>
> > > > > > > <a href="mailto:CRIU@openvz.org">CRIU@openvz.org</a><br>
> > > > > > > <a href="https://lists.openvz.org/mailman/listinfo/criu" rel="noreferrer" target="_blank">https://lists.openvz.org/<wbr>mailman/listinfo/criu</a><br>
> > > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > --<br>
> > > > > Best regards,<br>
> > > > > Eugene Batalov.<br>
> > > ><br>
> > ><br>
> > ><br>
> > ><br>
> > > --<br>
> > > Best regards,<br>
> > > Eugene Batalov.<br>
> ><br>
><br>
><br>
><br>
> --<br>
> Best regards,<br>
> Eugene Batalov.<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Best regards,<br>Eugene Batalov.</div>
</div>