* [PATCH] xen/credit2: Drop unnecessary bit test
@ 2018-01-11 16:48 Andrew Cooper
2018-01-11 16:50 ` George Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2018-01-11 16:48 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Dario Faggioli
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xen/credit2: Drop unnecessary bit test 2018-01-11 16:48 [PATCH] xen/credit2: Drop unnecessary bit test Andrew Cooper @ 2018-01-11 16:50 ` George Dunlap 2018-01-11 17:26 ` Dario Faggioli 0 siblings, 1 reply; 6+ messages in thread From: George Dunlap @ 2018-01-11 16:50 UTC (permalink / raw) To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Dario Faggioli 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/credit2: Drop unnecessary bit test 2018-01-11 16:50 ` George Dunlap @ 2018-01-11 17:26 ` Dario Faggioli 2018-01-11 17:36 ` Andrew Cooper 0 siblings, 1 reply; 6+ messages in thread From: Dario Faggioli @ 2018-01-11 17:26 UTC (permalink / raw) To: George Dunlap, Andrew Cooper, Xen-devel; +Cc: George Dunlap, Juergen Gross [-- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/credit2: Drop unnecessary bit test 2018-01-11 17:26 ` Dario Faggioli @ 2018-01-11 17:36 ` Andrew Cooper 2018-01-11 17:37 ` George Dunlap 2018-01-12 8:45 ` Dario Faggioli 0 siblings, 2 replies; 6+ messages in thread From: Andrew Cooper @ 2018-01-11 17:36 UTC (permalink / raw) To: Dario Faggioli, George Dunlap, Xen-devel; +Cc: George Dunlap, Juergen Gross 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/credit2: Drop unnecessary bit test 2018-01-11 17:36 ` Andrew Cooper @ 2018-01-11 17:37 ` George Dunlap 2018-01-12 8:45 ` Dario Faggioli 1 sibling, 0 replies; 6+ messages in thread From: George Dunlap @ 2018-01-11 17:37 UTC (permalink / raw) To: Andrew Cooper, Dario Faggioli, Xen-devel; +Cc: George Dunlap, Juergen Gross 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/credit2: Drop unnecessary bit test 2018-01-11 17:36 ` Andrew Cooper 2018-01-11 17:37 ` George Dunlap @ 2018-01-12 8:45 ` Dario Faggioli 1 sibling, 0 replies; 6+ messages in thread From: Dario Faggioli @ 2018-01-12 8:45 UTC (permalink / raw) To: Andrew Cooper, George Dunlap, Xen-devel; +Cc: George Dunlap, Juergen Gross [-- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-12 8:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-11 16:48 [PATCH] xen/credit2: Drop unnecessary bit test Andrew Cooper 2018-01-11 16:50 ` George Dunlap 2018-01-11 17:26 ` Dario Faggioli 2018-01-11 17:36 ` Andrew Cooper 2018-01-11 17:37 ` George Dunlap 2018-01-12 8:45 ` Dario Faggioli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).