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 -0000 1.122 > +++ uvm_page.c 26 Mar 2013 07:45:56 -0000 > @@ -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 >