Re: [PATCH] bpf: Simplify tnum_step()

[Paul Chaignon]

On Thu, Mar 19, 2026 at 10:01:31AM +0100, Hao Sun wrote:

Thanks for the cc Shung-Hsi. We were discussing it with Hari who's
planning to double check soundness.

> [...]
> > > -             res = w;
> > > -     }
> > > -     return res;
> > > +     d = z - t.value;
> >
> > A bit of comment explaining would be nice.
> >
> 
> The commit message is self-contained; anyone interested can git blame.
> But I am unsure. If a detailed description is preferred, I can add a
> comment to v2.

Not disagreeing about the role of git blame, but it's not a replacement
for code comments either. Having just the intuition in comments can help
a lot; as you mention below, it's one less indirection ;)

In its current form, I find the proof harder to follow than the
previous, very verbose version. Maybe there's a good middle ground?

> 
> > > +     carry_mask = (1ULL << fls64(d & ~t.mask)) - 1;
> > > +     inc = ((d | carry_mask | ~t.mask) + 1) & t.mask;
> >
> > Maybe break out the calculation of inc, give it a name (next_submask?),
> 
> Maybe we can break this down as:
> 
> d = z - t.value;
> carry_mask = (1ULL << fls64(d & ~t.mask)) - 1;
> filled = d | carry_mask | ~t.mask;
> inc = (filled + 1) & t.mask;
> 
> `next_submask` is not precise, as we ored `~t.mask` into it.
> It's always hard to name the intermediate result :)
> 
> > and have it as preceding patch? It seems generic enough that I thought
> > would have been implemented already, but bitmap_scatter() was the
> > closest I could find.
> >
> > Perhaps could be submitted to include/linux/bitops.h in the future.
> >
> 
> The calculation above is direct, adding a bitmap helper intros an indirection.
> For me, it means I would have to check out the meaning of the helper
> and then get back to understanding the algorithm.

I think I agree with Hao in this case. If we can reuse existing helpers,
great, but otherwise it's short enough that it's probably easier to keep
inlined here.