Re: [Patch v4 1/2] freezer: check OOM kill while being frozen

[Rafael J. Wysocki]

On Monday, September 08, 2014 10:40:17 AM Cong Wang wrote:
> On Fri, Sep 5, 2014 at 4:32 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> > On Saturday, September 06, 2014 07:45:54 AM Tejun Heo wrote:
> >> Hello,
> >>
> >> On Fri, Sep 05, 2014 at 11:12:24AM -0700, Cong Wang wrote:
> >> > > Rafael, can you please help?
> >> >
> >> > Rafael is known not responsive at least for this topic. :)
> >>
> >> :(
> >
> > Well, am I?
> >
> > I haven't commented patches in this thread so far, mostly because other
> > people have.
> >
> > How can I help actually?
> 
> 
> We asked you to comment on either if this patch is safe for PM freeze
> if we don't have the cgroup_freezing() check, or if it is not safe why (so that
> I can put it in the comment).

OK, which version of it?  Anything that has been posted as a complete patch you
have a link to?  Or am I supposed to go through the whole thread and figure out
how the patch *might* have looked had it been posted and then comment?

> >> > > Shouldn't the primary goal of the comment be explaining why we need
> >> > > TIF_MEMDIE check there at all anyway?  The deadlock possiblity is not
> >> > > very obvious.
> >> >
> >> > The changelog is not long enough?? ;-) I hate to copy+paste changelog
> >> > into comments, changelog is essentially necessary for people to understand
> >> > kernel code (at least networking) , so I don't think we have to move it
> >> > into comments in this case.
> >>
> >> It doesn't have to be the same text but the current comment is
> >> basically content-less.  e.g. it can just say "OOM killer may get
> >> stuck trying to kill a cgroup frozen task" and actualy provide
> >> information on what condition the conditional tries to address.
> >
> > Or something like "We need to check X to prevent Y from happening".
> >
> 
> OK, maybe just one or two sentences. Let me know if the following
> comment is okay for you:
> 
> /* OOM killer might decide to kill this process after it is frozen,
>   in this case it should thaw and die. */

I don't think it's sufficient.  In particular, what does "thaw and die" mean
exactly?  It should be thawed immediately and then die or it should die right
after it's thawed (at one point in the future)?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/