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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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.

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: signature.asc
Description: This is a digitally signed message part

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