On Wed, Jul 09, 2025 at 07:35:41AM -0700, Sean Christopherson wrote:
> On Wed, Jul 09, 2025, Neeraj Upadhyay wrote:
> > Use 'regs' as a contiguous linear bitmap for bitwise operations in
> > apic_{set|clear|test}_vector(). This makes the code simpler by eliminating
>
> That's very debatable. I don't find this code to be any simpler. Quite the
> opposite; it adds yet another open coded math exercise, which is so "simple"
> that it warrants its own comment to explain what it's doing.
>
> I'm not dead set against this, but I'd strongly prefer to drop this patch.
> > +static inline unsigned int get_vec_bit(unsigned int vec)
> > +{
> > + /*
> > + * The registers are 32-bit wide and 16-byte aligned.
> > + * Compensate for the resulting bit number spacing.
> > + */
> > + return vec + 96 * (vec / 32);
I kinda agree. The naked 96 doesn't tell me anything. If we do this, the
explaination of what this thing does should be crystal clear, perhaps even
with an example. And the naked numbers need to be defines with proper names.
Also:
> This change results in slight increase in generated code size for
> gcc-14.2.
>
> - Without change
What is the asm supposed to tell me?
The new change gets a LEA which is noticeable or so?
The generated code size increase is, what, a couple of bytes? Who cares?
We add asm to commit messages when it is really important. Doesn't seem so to
me here but maybe I'm missing an angle...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
