On Wed, Oct 23, 2024 at 9:46 AM Alan Huang <mmpgouride@xxxxxxxxx> wrote: > > 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. That condition is `if (srcu_idx < 0)`, no need for ugly macros that obscure things unnecessarily. > > > > > [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > >
