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 <[email protected]> > > --- > > CC: George Dunlap <[email protected]> > > CC: Dario Faggioli <[email protected]> > > > > 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/
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
