uvm combine clearbits
uvm_pagefree calls atomic_clearbits_int too many times. Just
accumulate the flags we need to zap, then do it once.
Index: uvm_page.c
===
RCS file: /cvs/src/sys/uvm/uvm_page.c,v
retrieving revision 1.122
diff -u -p -r1.122 uvm_page.c
--- uvm_page.c 12 Mar 2013 21:10:11 - 1.122
+++ uvm_page.c 26 Mar 2013 07:45:56 -
@@ -1053,6 +1053,7 @@ void
uvm_pagefree(struct vm_page *pg)
{
int saved_loan_count = pg-loan_count;
+ u_int flags_to_clear = 0;
#ifdef DEBUG
if (pg-uobject == (void *)0xdeadbeef
@@ -1115,7 +1116,7 @@ uvm_pagefree(struct vm_page *pg)
if (pg-pg_flags PQ_ACTIVE) {
TAILQ_REMOVE(uvm.page_active, pg, pageq);
- atomic_clearbits_int(pg-pg_flags, PQ_ACTIVE);
+ flags_to_clear |= PQ_ACTIVE;
uvmexp.active--;
}
if (pg-pg_flags PQ_INACTIVE) {
@@ -1123,7 +1124,7 @@ uvm_pagefree(struct vm_page *pg)
TAILQ_REMOVE(uvm.page_inactive_swp, pg, pageq);
else
TAILQ_REMOVE(uvm.page_inactive_obj, pg, pageq);
- atomic_clearbits_int(pg-pg_flags, PQ_INACTIVE);
+ flags_to_clear |= PQ_INACTIVE;
uvmexp.inactive--;
}
@@ -1138,15 +1139,16 @@ uvm_pagefree(struct vm_page *pg)
if (pg-uanon) {
pg-uanon-an_page = NULL;
pg-uanon = NULL;
- atomic_clearbits_int(pg-pg_flags, PQ_ANON);
+ flags_to_clear |= PQ_ANON;
}
/*
* Clean page state bits.
*/
- atomic_clearbits_int(pg-pg_flags, PQ_AOBJ); /* XXX: find culprit */
- atomic_clearbits_int(pg-pg_flags, PQ_ENCRYPT|
- PG_ZERO|PG_FAKE|PG_BUSY|PG_RELEASED|PG_CLEAN|PG_CLEANCHK);
+ flags_to_clear |= PQ_AOBJ; /* XXX: find culprit */
+ flags_to_clear |= PQ_ENCRYPT|PG_ZERO|PG_FAKE|PG_BUSY|PG_RELEASED|
+ PG_CLEAN|PG_CLEANCHK;
+ atomic_clearbits_int(pg-pg_flags, flags_to_clear);
/*
* and put on free queue
Re: uvm combine clearbits
On Tue, Mar 26, 2013 at 1:51 AM, Ted Unangst t...@tedunangst.com wrote:
uvm_pagefree calls atomic_clearbits_int too many times.
Is there some sort of evidence that this is a problem - performace or
stability wise?
Just
accumulate the flags we need to zap, then do it once.
I get what you're trying to do, but given that there are already
enough cases of this in that code and you're now just making a few
special cases of saving the flag to avoid a few calls I don't think
it's worth the added cleverness in code that people like me have to
spend a lot of time wading through looking for errors - particularly
the types of errors that involve can this sleep or not... So I
don't personaly think this turdshining is worth it. I would
rather just see the bits set and know they are set just as in every other case.
Index: uvm_page.c
===
RCS file: /cvs/src/sys/uvm/uvm_page.c,v
retrieving revision 1.122
diff -u -p -r1.122 uvm_page.c
--- uvm_page.c 12 Mar 2013 21:10:11 - 1.122
+++ uvm_page.c 26 Mar 2013 07:45:56 -
@@ -1053,6 +1053,7 @@ void
uvm_pagefree(struct vm_page *pg)
{
int saved_loan_count = pg-loan_count;
+ u_int flags_to_clear = 0;
#ifdef DEBUG
if (pg-uobject == (void *)0xdeadbeef
@@ -1115,7 +1116,7 @@ uvm_pagefree(struct vm_page *pg)
if (pg-pg_flags PQ_ACTIVE) {
TAILQ_REMOVE(uvm.page_active, pg, pageq);
- atomic_clearbits_int(pg-pg_flags, PQ_ACTIVE);
+ flags_to_clear |= PQ_ACTIVE;
uvmexp.active--;
}
if (pg-pg_flags PQ_INACTIVE) {
@@ -1123,7 +1124,7 @@ uvm_pagefree(struct vm_page *pg)
TAILQ_REMOVE(uvm.page_inactive_swp, pg, pageq);
else
TAILQ_REMOVE(uvm.page_inactive_obj, pg, pageq);
- atomic_clearbits_int(pg-pg_flags, PQ_INACTIVE);
+ flags_to_clear |= PQ_INACTIVE;
uvmexp.inactive--;
}
@@ -1138,15 +1139,16 @@ uvm_pagefree(struct vm_page *pg)
if (pg-uanon) {
pg-uanon-an_page = NULL;
pg-uanon = NULL;
- atomic_clearbits_int(pg-pg_flags, PQ_ANON);
+ flags_to_clear |= PQ_ANON;
}
/*
* Clean page state bits.
*/
- atomic_clearbits_int(pg-pg_flags, PQ_AOBJ); /* XXX: find culprit */
- atomic_clearbits_int(pg-pg_flags, PQ_ENCRYPT|
- PG_ZERO|PG_FAKE|PG_BUSY|PG_RELEASED|PG_CLEAN|PG_CLEANCHK);
+ flags_to_clear |= PQ_AOBJ; /* XXX: find culprit */
+ flags_to_clear |= PQ_ENCRYPT|PG_ZERO|PG_FAKE|PG_BUSY|PG_RELEASED|
+ PG_CLEAN|PG_CLEANCHK;
+ atomic_clearbits_int(pg-pg_flags, flags_to_clear);
/*
* and put on free queue
Re: uvm combine clearbits
uvm_pagefree calls atomic_clearbits_int too many times. Is there some sort of evidence that this is a problem - performace or stability wise? Platforms which can't do ll/sc style atomic operations usually wrap these operations within splhigh()/splx() calls, which are a tad expensive. In that particular diff, Ted makes sure to flip the bits only during the time the vm_page is not on any TAILQ, and before it is put on the free list. This is narrow enough to me.
Re: uvm combine clearbits
uvm_pagefree calls atomic_clearbits_int too many times. Is there some sort of evidence that this is a problem - performace or stability wise? Platforms which can't do ll/sc style atomic operations usually wrap these operations within splhigh()/splx() calls, which are a tad expensive. In that particular diff, Ted makes sure to flip the bits only during the time the vm_page is not on any TAILQ, and before it is put on the free list. This is narrow enough to me. Same opinion.
Re: uvm combine clearbits
On Tue, Mar 26, 2013 at 10:55 AM, Miod Vallat m...@online.fr wrote: uvm_pagefree calls atomic_clearbits_int too many times. Is there some sort of evidence that this is a problem - performace or stability wise? Platforms which can't do ll/sc style atomic operations usually wrap these operations within splhigh()/splx() calls, which are a tad expensive. In that particular diff, Ted makes sure to flip the bits only during the time the vm_page is not on any TAILQ, and before it is put on the free list. This is narrow enough to me. Yeah, ok, in light of that explanation I'm better with this. No objections here then.
