Re: [PATCH v2 8/9] x86/HVM: drop stdvga's "lock" struct member

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2024 15:07, Andrew Cooper wrote:
> On 11/09/2024 1:58 pm, Jan Beulich wrote:
>> On 11.09.2024 14:42, Andrew Cooper wrote:
>>> On 11/09/2024 1:29 pm, Jan Beulich wrote:
>>> However for performance, we also want to do the dir/ptr/count exclusions
>>> before the address range exclusions, meaning that ...
>>>
>>>>  
>>>> -    spin_lock(&s->lock);
>>>> -
>>>>      if ( p->dir != IOREQ_WRITE || p->data_is_ptr || p->count != 1 )
>>>>      {
>>>>          /*
>>>>           * Only accept single direct writes, as that's the only thing we 
>>>> can
>>>>           * accelerate using buffered ioreq handling.
>>>>           */
>>> ... it wants merging with this into a single expression.
>> I'm not convinced, and hence would at least want to keep this separate.
>> Which exact order checks want doing in would require more detailed
>> analysis imo. Or do you have blindingly obvious reasons to believe that
>> the re-ordering you suggest is always going to be an improvement?
> 
> I'm not overly fussed if this is delayed to a later patch.  My review
> stands as long as the comment is gone.
> 
> But, right now, accept() is called linearly over all handlers (there's
> not range based registration) so *every* IO comes through this logic path.

Not exactly every, only ones not claimed earlier. But yes.

> The likely path is the excluded path.  ioreq_mmio_{first,last}_byte()
> are non-trivial logic because they account for DF, so being able to
> exclude based on direction/size before the DF calculations is a definite
> improvement.

Perhaps. Yet if we were to re-order, calling ioreq_mmio_{first,last}_byte()
becomes questionable in the first place. I wouldn't expect the compiler to
spot that it can reduce those expressions as a result of knowing ->count
being 1 (and hence ->df playing no role at all). Maybe I'm overly
pessimistic ...

Jan