Re: [Xen-devel] [PATCH] xen/credit2: Drop unnecessary bit test

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/01/18 17:26, Dario Faggioli wrote:
> On Thu, 2018-01-11 at 16:50 +0000, George Dunlap wrote:
>> On 01/11/2018 04:48 PM, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>>> CC: Dario Faggioli <dfaggioli@xxxxxxxx>
>>>
>>> Notices by chance while inspecting the disassembly delta for
>>> "x86/bitops:
>>> Introduce variable/constant pairs for __{set,clear,change}_bit()"
>>> ---
>>>  xen/common/sched_credit2.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/common/sched_credit2.c
>>> b/xen/common/sched_credit2.c
>>> index 18f39ca..ee9768e 100644
>>> --- a/xen/common/sched_credit2.c
>>> +++ b/xen/common/sched_credit2.c
>>> @@ -2063,7 +2063,7 @@ csched2_vcpu_sleep(const struct scheduler
>>> *ops, struct vcpu *vc)
>>>          update_load(ops, svc->rqd, svc, -1, NOW());
>>>          runq_remove(svc);
>>>      }
>>> -    else if ( svc->flags & CSFLAG_delayed_runq_add )
>>> +    else
>>>          __clear_bit(__CSFLAG_delayed_runq_add, &svc->flags);
>> There was a reason for this at some point, I'm sure.  
>>
> Adding Juergen, as commit e8abdea48a ("use masking operation instead of
> test_bit for CSFLAG bits") is his.
>
>> Did this used to
>> be the atomic version (without the __) originally?
>>
> At the beginning, yes. In fact, if you look at how the code was before
> Juergen's patch:
>
>     else if ( test_bit(__CSFLAG_delayed_runq_add, &svc->flags) )
>          clear_bit(__CSFLAG_delayed_runq_add, &svc->flags);
>
> Which indeed was overkill. That patch got rid of test_bit(), but did
> not touch clear_bit().
>
> I then turned the clear_bit() in __clear_bit() in commit 222234f2ad
> ("xen: credit2: use non-atomic cpumask and bit operations") but kept
> the test.
>
> From a code readability perspective, I like this patch (and have
> thought about doing this myself many times). From a performance
> perspective, the test may make sense. In fact, we do a technically
> unnecessary "load", but that may avoid having to pay the price of a
> "store".
>
> I guess it's debatable whether that is worth or not, in general.
> However, at least in this specific case, I don't think this matters too
>  much, and I'd be inclined to take the patch.

It is generally worth doing a read to conditionally avoid a locked RMW,
in the case that you expect the locked RMW to be unnecessary (i.e. the
modification is already present).

The same is not true for plain memory reads and writes.  The overhead of
the conditional jump far outweighs the saving of possibly not dirtying
the cache line.

The reason I noticed this is because (with my bitops change), the
compiler optimised the if out entirely.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel