On Tue, Jan 20, 2026 at 11:22:45AM -0400, Jason Gunthorpe wrote:
> On Tue, Jan 20, 2026 at 03:10:54PM +0000, Lorenzo Stoakes wrote:
> > The natural implication of what you're saying is that we can no longer use
> > this
> > from _anywhere_ because - hey - passing this by value is bad so now
> > _everything_
> > has to be re-written as:
>
> No, I'm not saying that, I'm saying this specific case where you are
> making an accessor to reach an unknown value located on the heap
OK it would have been helpful for you to say that! Sometimes reviews feel
like a ratcheting series of 'what do you actually mean?'s... :)
> should be using a pointer as both a matter of style and to simplify
> life for the compiler.
OK fine.
>
> > vma_flags_t flags_to_set = mk_vma_flags(<flags>);
> >
> > if (vma_flags_test(&flags, &flags_to_set)) { ... }
>
> This is quite a different situation, it is a known const at compile
> time value located on the stack.
Well as a const time thing it'll be optimised to just a value assuming
nothing changes flags_to_set in the mean time. You'd hope.
Note that we have xxx_mask() variants, such that you can do, e.g.:
vma_flags_t flags1 = mk_vma_flags(...);
vma_flags_t flags2 = mk_vma_flags(...);
if (vma_flags_test_mask(flags1, flags2)) {
...
}
ASIDE ->
NOTE: A likely use of this, and one I already added is so we can do
e.g.:
#define VMA_REMAP_FLAGS mk_vma_flags(VMA_IO_BIT, VMA_PFNMAP_BIT, \
VMA_DONTEXPAND_BIT, VMA_DONTDUMP_BIT)
...
if (vma_flagss_test_mask(flags, VMA_REMAP_FLAGS)) { ... }
Which would be effectively a const input anyway.
<- ASIDE
Or in a world where flags1 is a const pointer now:
if (vma_flags_test_mask(&flags1, flags2)) { ... }
Which makes the form... kinda weird. Then again it's consistent with other
forms which update flags1, ofc we name this separately, e.g. flags, to_test
or flags, to_set so I guess not such a problme.
Now, nobody is _likely_ to do e.g.:
if (vma_flags_test_mask(&vma1->flags, vma2->flags)) { ... }
In this situation, but they could.
However perhaps having one value pass-by-const-pointer and the other
by-value essentially documents the fact you're being dumb.
And if somebody really needs something like this (not sure why) we could
add something.
But yeah ok, I'll change this. It's more than this case it's also all the
test stuff but shouldn't be a really huge change.
>
> > If it was just changing this one function I'd still object as it makes it
> > differ
> > from _every other test predicate_ using vma_flags_t but maybe to humour you
> > I'd
> > change it, but surely by this argument you're essentially objecting to the
> > whole
> > series?
>
> I only think that if you are taking a heap input that is not of known
> value you should continue to pass by pointer as is generally expected
> in the C style we use.
Ack.
>
> And it isn't saying anything about the overall technique in the
> series, just a minor note about style.
OK good, though Arnd's reply feels more like a comment on the latter,
though only really doing pass-by-value for const values (in nearly all sane
cases) should hopefully mitigate.
>
> > I am not sure about this 'idiomatic kernel style' thing either, it feels
> > rather
> > conjured. Yes you wouldn't ordinarily pass something larger than a register
> > size
> > by-value, but here the intent is for it to be inlined anyway right?
>
> Well, exactly, we don't normally pass things larger than an interger
> by value, that isn't the style, so I don't think it is such a great
> thing to introduce here kind of unnecessarily.
>
> The troubles I recently had were linked to odd things like gcov and
> very old still supported versions of gcc. Also I saw a power compiler
> make a very strange choice to not inline something that evaluated to a
> constant.
Right ok.
>
> Jason
Thanks, Lorenzo
