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

[Alan Huang <mmpgouride@gmail.com>]

On Oct 24, 2024, at 00:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Tue, Oct 22, 2024 at 11:40 PM Christoph Hellwig <hch@infradead.org> 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@noisy.programming.kicks-ass.net/