On Tue, Mar 26, 2013 at 1:51 AM, Ted Unangst <[email protected]> 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
>
