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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/11/2018 05:36 PM, Andrew Cooper wrote:
> 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.

Yes, if it's not necessary we might as well remove it.

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

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