[PATCH 4/8] nptl: Add rseq registration

[Siddhesh Poyarekar]

On 12/9/21 13:12, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> On 12/8/21 23:38, Florian Weimer wrote:
>>> * Siddhesh Poyarekar:
>>>
>>>>> +#ifdef RSEQ_SIG
>>>>> +static inline void
>>>>> +rseq_register_current_thread (struct pthread *self)
>>>>> +{
>>>>> +  int ret = INTERNAL_SYSCALL_CALL (rseq,
>>>>> +                                   &self->rseq_area, sizeof (self->rseq_area),
>>>>> +                                   0, RSEQ_SIG);
>>>>> +  if (INTERNAL_SYSCALL_ERROR_P (ret))
>>>>> +    THREAD_SETMEM (self, rseq_area.cpu_id, RSEQ_CPU_ID_REGISTRATION_FAILED);
>>>>
>>>> Why can't we just leave it as the kernel did when it failed the
>>>> syscall?
>>> The kernel definitely won't write anything if the failure is ENOSYS.
>>> I
>>> don't expect the kernel to write something for the other failures,
>>> either.
>>
>> OK, I interpreted the from the outdated manpage patch[1] that the
>> kernel ensures that uninitialized cpu_id will be read as -1.  I read
>> the rseq implementation in the kernel and saw that there are a number
>> of error paths where the kernel simply returns without touching the
>> user memory.   I suppose what they meant by "uninitialized" in the
>> manpage is actually "reset after unregister", which is odd.
>>
>> In any case, what I meant to eventually get at (sorry I wasn't
>> specific; I wrote both patch reviews together and didn't realize
>> they'd be read as separate emails!) is that RSEQ_CPU_ID_UNINITIALIZED
>> seemed enough for all use cases and RSEQ_CPU_ID_REGISTRATION_FAILED
>> seemed unnecessary.
> 
> Yes, but the constant is (also) defined in the UAPI headers, so it's
> value is fixed.  And RSEQ_CPU_ID_REGISTRATION_FAILED (that is, -2)
> is closer to the behavior we want to trigger in application (that there
> is nothing to register because we already tried and failed).

OK, I see it in the headers, sorry.  Once again I assumed only 
RSEQ_CPU_ID_UNINITIALIZED was defined because the man page didn't 
specify it :/

It's redundant IMO, but that's a Linux API problem.  No objections from 
me then.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>