On 28/08/2024 9:13 am, Jan Beulich wrote:
> On 27.08.2024 15:57, Andrew Cooper wrote:
>> In the HPET_STATUS handling, the use of __clear_bit(i, &new_val) is the only
>> thing causing it to be spilled to the stack. Furthemore we only care about
>> the bottom 3 bits, so rewrite it to be a plain for loop.
>>
>> For the {start,stop}_timer variables, these are spilled to the stack despite
>> the __{set,clear}_bit() calls.
> That's an observation from what the compiler happens to do? I don't see any
> other reason why they would need spilling; I expect it's merely a matter of
> registers better be used for other variables.
It is a consequence of how our helpers are written. I do expect it to
improve when I get around to reworking them.
For example, the Linux helpers have enough constant folding capabilities
to allow the compiler to turn:
{
int foo = 0;
...
__set_bit(1, &foo);
into:
{
int foo = 1;
as well as being able to emit LOCK AND/OR/XOR in place of LOCK BT{C,S,R}
for a constant bit position.
One thing I want to do, which I haven't figured out how to do yet, is to
allow the arch form to emit BT?Q forms.
Right now, code generation for PGC_* and PGT_* suffers quite a lot. We
mix between reg/imm logic, then spill to the stack because top bits
aren't within range for the "I" constraint on 32-bit instructions, issue
a BT?L reg/mem (which has much higher latency than any other form), then
pick it back off the stack to do more reg/imm logic.
I was wondering if, because of the always_inline, I could do something
like __builtin_constant_p(bit) && __builtin_object_size(addr, 0) >= 8
and emitting long-granular logic, which will be able to pick the imm/reg
form rather than turning into reg/mem.
But, I've not had time to experiment here, and I doubt I'll get around
to it soon.
Another optimisation we're lacking vs Linux is that our test_bit() has a
volatile pointer where Linux's is non-volatile. This makes a massive
difference for the ability to optimise looking at multiple bits.
> If we ever meant to build Xen
> with APX fully in use, that might change. IOW may I at least ask for
> s/are/happen to be/? I'm also a little irritated by "despite", but you're
> the native speaker. It would have seemed to me that e.g. "irrespective of"
> would better express what (I think) is meant.
"despite" isn't really the right term, but I also wouldn't have said it
was something to be irritated over.
What I was trying to say was "they're spilled to the stack even with the
__set_bit() calls removed". Which makes sense; they're values held for
almost the full duration of the function, that are not used in ~every
step of logic.
Interestingly, given that they're spilled to the stack, the __set_bit()
form is more efficient than the plain C "|= (1u << i);", but I'd still
like an implementation which could make that determination itself.
>
>> Again we only care about the bottom 3 bits, so
>> shrink the variables from long to int. Use for_each_set_bit() rather than
>> opencoding it at the end which amongst other things means the loop predicate
>> is no longer forced to the stack by the loop body.
>>
>> Signed-off-by: Andrew Cooper <[email protected]>
>> ---
>> CC: Jan Beulich <[email protected]>
>> CC: Roger Pau Monné <[email protected]>
>>
>> All in all, it's modest according to bloat-o-meter:
>>
>> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
>> Function old new delta
>> hpet_write 2225 2196 -29
>>
>> but we have shrunk the stack frame by 8 bytes; 0x28 as opposed to 0x30
>> before.
> However, on the negative side all the first of the loops you touch now always
> takes 3 iterations, when previously we may have got away with as little as
> none. Is there a reason not to use
>
> for_each_set_bit ( i, new_val & ((1U << HPET_TIMER_NUM) - 1) )
>
> there (with the masking of the low bit possibly pulled out)?
There are multiple angles here.
First, I got an unexpected surprise on ARM with an expression, and while
this one won't pick up pointer const-ness, I can never remember what
MISRA's view on this is.
Second, this is the odd-loop-out compared to rest of the function, which
are all of the form "for ( i = 0; i < HPET_TIMER_NUM ;".
But perhaps most importantly, OSes don't touch this register. Xen not
at all, and Linux only in _hpet_print_config(). Neither bother
preserving/clearing it on suspend/resume, even when running the HPET in
legacy replacement mode.
I haven't checked windows behaviour, but I don't expect it to differ
here. This register simply isn't interesting for the preferred type of
interrupts (edge), and also isn't useful for an ISR handling a line
interrupt.
So my choice was based on which produced the smallest code, because it's
an dead-in-practice codepath.
>
>> @@ -533,19 +528,11 @@ static int cf_check hpet_write(
>> }
>>
>> /* stop/start timers whos state was changed by this write. */
>> - while (stop_timers)
>> - {
>> - i = ffsl(stop_timers) - 1;
>> - __clear_bit(i, &stop_timers);
>> + for_each_set_bit ( i, stop_timers )
>> hpet_stop_timer(h, i, guest_time);
>> - }
>>
>> - while (start_timers)
>> - {
>> - i = ffsl(start_timers) - 1;
>> - __clear_bit(i, &start_timers);
>> + for_each_set_bit ( i, start_timers )
>> hpet_set_timer(h, i, guest_time);
>> - }
> To avoid variable shadowing, I think you don't want to use i in these two
> loops. Alternatively the function scope i would need constraining to the
> individual loops.
Yeah, I was bitten by that on one of the ARM patches. I'll adjust.
~Andrew
