[PATCH] xen/credit2: Drop unnecessary bit test

[PATCH] xen/credit2: Drop unnecessary bit test

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Dario Faggioli <dfaggioli@suse.com>

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);
 }
 
-- 
2.1.4


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

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

[George Dunlap]

On 01/11/2018 04:48 PM, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Dario Faggioli <dfaggioli@suse.com>
> 
> 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.  Did this used to
be the atomic version (without the __) originally?

 -George

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2519 bytes --]

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@citrix.com>
> > ---
> > CC: George Dunlap <george.dunlap@eu.citrix.com>
> > CC: Dario Faggioli <dfaggioli@suse.com>
> > 
> > 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.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Andrew Cooper]

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@citrix.com>
>>> ---
>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>> CC: Dario Faggioli <dfaggioli@suse.com>
>>>
>>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[George Dunlap]

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@citrix.com>
>>>> ---
>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>> CC: Dario Faggioli <dfaggioli@suse.com>
>>>>
>>>> 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@citrix.com>

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

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 964 bytes --]

On Thu, 2018-01-11 at 17:36 +0000, Andrew Cooper wrote:
> 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.
> 
Sure.

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

It's not necessary, as George Acked it already, but feel free to add a:

Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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