Re: [PATCH 5/7] linux: Use waitid on wait4 if __NR_wait4 is not defined

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 22/11/2019 17:00, Alistair Francis wrote:
> On Fri, Nov 22, 2019 at 4:15 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 21/11/2019 15:41, Alistair Francis wrote:
>>> On Thu, Nov 21, 2019 at 9:53 AM Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 14/11/2019 11:47, Adhemerval Zanella wrote:
>>>>> +pid_t
>>>>> +__wait4 (pid_t pid, int *stat_loc, int options, struct rusage *usage)
>>>>> +{
>>>>> +#if __NR_wait4
>>>>> +   return SYSCALL_CANCEL (wait4, pid, stat_loc, options, usage);
>>>>> +#elif defined (__ASSUME_WAITID_PID0_P_PGID)
>>>> [...]
>>>>> +# else
>>>>> +/* Linux waitid prior kernel 5.4 does not support waiting for the current
>>>>> +   process.  It would be possible to emulate it by calling getpgid for pid 0,
>>>>> +   however, it would require an additional syscall and it is inherent racy:
>>>>> +   after the current process group is received and before it is passed
>>>>> +   to waitid a signal could arrive causing the current process group to
>>>>> +   change.  */
>>>>> +# error "The kernel ABI does not provide a way to implement wait4"
>>>>> +#endif
>>>>
>>>> So the only design here that I am not sure is if the best one is to trigger
>>>> a build error to avoid an architecture to not define __NR_wait4 and also
>>>> support kernels older than 5.4 (which would not define
>>>> __ASSUME_WAITID_PID0_P_PGID), or if it should do as generic implementation
>>>> and return ENOSYS along with a stub.
>>>>
>>>> Thoughts?
>>>
>>> I think a build error makes sense. Currently only RV32 doesn't have
>>> __NR_wait4 (which isn't upstreamed) so you aren't breaking anything.
>>>
>>> The only kernels that could possibly not have __NR_wait4 and be less
>>> then 5.4 are 5.1, 5.2 and 5.3, non of which are stable so they will
>>> slowly disappear anyway.
>>>
>>> Not producing a build error could be very confusing for developers
>>> that do get bitten by the missing implementation.
>>>
>>
>> My point if if checking for kernel version to define __ASSUME_WAITID_PID0_P_PGID
>> does make, meaning it is possible with some config option in the kernel
>> to enable only waitid for kernels older than 5.3; or if we can assume
>> some configuration in always invalid and thus the kernel won't allow
>> enable it.
>>
>> If the latter we can then remove the __ASSUME_WAITID_PID0_P_PGID and
>> add a comment on waitid implementation stating that if waitid is the
>> only syscall supported then it is suppose to be the superset of all
>> wait* functionalities.
> 
> I think I understand what you are saying.
> 
> It is NOT the case that if waitid is the only syscall supported then
> it is a superset of all wait* functions.
> 
> For RV32 the 5.1, 5.2 and 5.3 only support waitid but do not support
> the PID0 P_PGID functionality. In these three kernel cases the call
> will fail.
> 

If it is the case (and it does seems linux support RV32 for kernel older
than v5.4), we will need to make the 5.4 the minimum supported kernel for
RV32 and any other architecture that only support waitid syscall.

The kernel check can be done at the configure level, but it does not
help on some kernel configuration for a architecture that explicit 
try to remove olds syscall to mimic the generic user API (not sure if
this is possible with current kernel configuration flags).

So it seems that __ASSUME_WAITID_PID0_P_PGID should be a safe call to
avoid such theoretical scenarios.  I will rework how the flag is used
to mimic other assume usage where newer kernel version undef the flag,
so it should be simpler to remove it once the minimum kernel is lifted.