Re: [PATCH rcu] srcu: Guarantee non-negative return value from srcu_read_lock()

From: Alan Huang
Date: Wed Oct 23 2024 - 12:46:56 EST


On Oct 24, 2024, at 00:34, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Oct 22, 2024 at 11:40 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>>
>> On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote:
>>>>
>>>> Would this work?
>>>>
>>>> #define SRCU_INVALID_INDEX -1
>>>>
>>>
>>> But why?
>>
>> Becaue it very clearly documents what is going on.
>
> So does saying "returned value is going to be non-negative, always".
> It's not some weird and unusual thing in C by any means.
>
>>
>>> It's a nice property to have an int-returning API where valid
>>> values are only >= 0, so callers are free to use the entire negative
>>> range (not just -1) for whatever they need to store in case there is
>>> no srcu_idx value.
>>
>> Well, if you have a concrete use case for that we can probably live
>> with it, but I'd rather have that use case extremely well documented,
>> as it will be very puzzling to the reader.
>>
>
> See [0] for what Peter is proposing. Note hprobe_init() and its use of
> `srcu_idx < 0` as a mark that there is no SRCU "session" associated
> with the uprobe. Now, if we say that SRCU_INVALID_INDEX is the only
> invalid value, it leaves a question: what to do with other negative
> srcu_idx values? Are they valid? Should I now WARN() on "unknown
> invalid" values? Why all these complications? I'd rather just not have
> a unified hprobe_init() at that point, TBH.
>
> But anyways, taking a step back. Take idr_alloc()/idr_alloc_cyclic()
> APIs as an example. They return int, but valid IDs are documented to
> be positive. This leaves users of this API free to use int to store ID
> in their data structures, knowing that <= 0 is "no ID", and if
> necessary, they can have multiple possible "no ID" situations.
>
> Same principle here. Why prescribing a randomly picked -1 as the only
> "valid" invalid value, if anything negative is clearly impossible.
>

A IS_INVALID_SRCU_INDEX macro would suffice, you need the condition anyway.

>
> [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/