Re: [patch 18/39] mm/madvise: check fatal signal pending of target process

[Minchan Kim <minchan@kernel.org>]

On Sat, Aug 15, 2020 at 06:43:08PM -0700, Linus Torvalds wrote:
> On Sat, Aug 15, 2020 at 11:35 AM Minchan Kim <minchan@kernel.org> wrote:
> >
> > > Now, it might be worth it to have some kind of "this mm is dying,
> > > don't bother" thing. We _kind_ of have things like that already in the
> > > form of the MMF_OOM_VICTIM flag (and TIF_MEMDIE is the per-thread
> > > image of it).
> >
> > Maybe, we could use mm_struct's mm_users to check caller is exclusive
> > owner so that rest of all are existing.
> 
> Hmm. Checking mm_users sounds sane. But I think the /proc reference by
> any get_task_mm() site will also count as a mm_user, so it's not quite
> as useful as it could be.
> 
> In an optimal world, all the temporary "grab a reference to the mm"
> would use mmgrab/mmdrop() that increments the mm_count, and "mm_users"
> would mean the number of actual threads that are actively using it.
> 
> But that's not how it ends up working. mmgrab/mmdrop only keeps the
> "struct mm_struct" around - it doesn't keep the vma's or the page
> tables. So all the /proc users really do want to increase mm_users.
> 
> I don't see any obvious thing to check for that would be about "this
> mm no longer makes sense to madvise on, because nobody cares any
> more".

Yeah, there are bunch of places where makes false negaive potentially
as well as proc but I expected it would be rather rare and even though
it happens, finally, we can catch it up if they are temporally holding
the refcount but our operation runs long.
At worst case, it could make the operation void so we just wastes but
when I consider the logic as optimization, it wouldn't be harmful to
start with such *simple check* rather than adding more complication.

If you still don't like the idea, at this point, I will drop the single
patch as I mentioned because I don't think I have strong justification
to add more complication here.

> 
> > Please tell me if you found something weird in this patchset series
> > so that in next cycle we could go smooth.
> 
> No, the only other thing that worried me was just possible locking,
> but it looked like we already have all the "access page tables from
> other processes" situations and it didn't seem to introduce anything
> new in that respect.

Thanks for the review, Linus.