Re: [PATCH v7 5/7] mm: support both pid and pidfd for process_madvise

[Minchan Kim <minchan@kernel.org>]

Hi Christian,

On Sat, May 09, 2020 at 02:48:17PM +0200, Christian Brauner wrote:
> On Fri, May 08, 2020 at 04:04:15PM -0700, Andrew Morton wrote:
> > On Fri, 8 May 2020 11:36:53 -0700 Minchan Kim <minchan@kernel.org> wrote:
> > 
> > > 
> > > ...
> > >
> > > Per Vlastimil's request, I changed "which and advise" with "idtype and
> > > advice" in function prototype of description.
> > > Could you replace the part in the description? Code is never changed.
> > > 
> > 
> > Done, but...
> > 
> > >
> > > ...
> > >
> > > There is a demand[1] to support pid as well pidfd for process_madvise to
> > > reduce unnecessary syscall to get pidfd if the user has control of the
> > > target process(ie, they could guarantee the process is not gone or pid is
> > > not reused).
> > > 
> > > This patch aims for supporting both options like waitid(2).  So, the
> > > syscall is currently,
> > > 
> > >         int process_madvise(idtype_t idtype, id_t id, void *addr,
> > >                 size_t length, int advice, unsigned long flags);
> > > 
> > > @which is actually idtype_t for userspace libray and currently, it
> > > supports P_PID and P_PIDFD.
> > 
> > What does "@which is actually idtype_t for userspace libray" mean?  Can
> > you clarify and expand?
> 
> If I may clarify, the only case where we've supported both pidfd and pid
> in the same system call is waitid() to avoid adding a dedicated system
> call for waiting and because waitid() already had this (imho insane)
> argument type switching. The idtype_t thing comes from waitid() and is
> located int sys/wait.h and is defined as
> 
> "The type idtype_t is defined as an enumeration type whose possible
> values include at least the following:
> 
> P_ALL
> P_PID
> P_PGID
> "
> 
> int waitid(idtype_t idtype, id_t id, siginfo_t *infop, int options);
> If idtype is P_PID, waitid() shall wait for the child with a process ID equal to (pid_t)id.
> If idtype is P_PGID, waitid() shall wait for any child with a process group ID equal to (pid_t)id.
> If idtype is P_ALL, waitid() shall wait for any children and id is ignored.
> 
> I'm personally not a fan of this idtype_t thing and think this should
> just have been 
> > >         int pidfd_madvise(int pidfd, void *addr,
> > >                 size_t length, int advice, unsigned long flags);
> and call it a day.

That was the argument at that time, Daniel and I didn't want to have
pid along with pidfd even though Kirill strongly wanted to have it.
However you said " Overall, I don't particularly care how or if you
integrate pidfd here." at that time.

https://lore.kernel.org/linux-mm/20200113104256.5ujbplyec2sk4onn@wittgenstein/

I asked a question to Kirll at that time.

"
> Sounds like that you want to support both options for every upcoming API
> which deals with pid. I'm not sure how it's critical for process_madvise
> API this case. In general, we sacrifice some performance for the nicer one
> and later, once it's reported as hurdle for some workload, we could fix it
> via introducing new flag. What I don't like at this moment is to make
> syscall complicated with potential scenarios without real workload.

Yes, I suggest allowing both options for every new process api
"
https://lore.kernel.org/linux-mm/9d849087-3359-c4ab-fbec-859e8186c509@virtuozzo.com/

You didn't give the opinion at that time, either(I expected you will
make some voice then). What I could do to proceed work was separate it
as different patch like this one to get more attention in future.
And now it works.

Let me clarify my side: I still don't like to introduce pid for new API
since we have pidfd. Since you just brought this issue again, I want to
hear *opinions* from others, again.

> 
> Also, if I may ask, why is the flag argument "unsigned long"?
> That's pretty unorthodox. The expectation is that flag arguments are
> not word-size dependent and should usually use "unsigned int". All new
> system calls follow this pattern too.

Nothing special in this flag: Let me change it as "unsigned int".
I will send the change once we have an agreement on "pidfd" argument.

Thanks for the review, Christian!