Re: [PATCH 1/4] mm/gup: Add FOLL_INTERRUPTIBLE

[John Hubbard]

On 6/28/22 15:33, Peter Xu wrote:
> > The key point is the connection between "locked" and killable. If the comment
> > explained why "locked" means "killable", that would help clear this up. The
> > NOWAIT sentence is also confusing to me, and adding "mostly NOWAIT" does not
> > clear it up either... :)
>
> Sorry to have a comment that makes it feels confusing.  I tried to
> explicitly put the comment to be after setting FAULT_FLAG_KILLABLE but
> obviously I didn't do my job well..
>
> Maybe that NOWAIT thing adds more complexity but not even necessary.
>
> Would below one more acceptable?
>
> 		/*
> 		 * We'll only be able to respond to signals when "locked !=
> 		 * NULL".  When with it, we'll always respond to SIGKILL
> 		 * (as implied by FAULT_FLAG_KILLABLE above), and we'll
> 		 * respond to non-fatal signals only if the GUP user has
> 		 * specified FOLL_INTERRUPTIBLE.
> 		 */


It looks like part of this comment is trying to document a pre-existing
concept, which is that faultin_page() only ever sets FAULT_FLAG_KILLABLE
if locked != NULL. The problem I am (personally) having is that I don't
yet understand why or how those are connected: what is it about having
locked non-NULL that means the process is killable? (Can you explain why
that is?)

If that were clear, I think I could suggest a good comment wording.




thanks,
--
John Hubbard
NVIDIA