Date: Tue, 12 Mar 2013 17:34:56 +0100 From: Attilio Rao <attilio@freebsd.org> To: Alan Cox <alc@rice.edu> Cc: Alan Cox <alc@freebsd.org>, src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r248185 - user/attilio/vmcontention/sys/vm Message-ID: <CAJ-FndDvoub2mXRj-orjwS2dH_qfhKPSBE7q5hbNJAtKbQJM-g@mail.gmail.com> In-Reply-To: <513F571A.5070007@rice.edu> References: <201303120614.r2C6EWve058965@svn.freebsd.org> <CAJ-FndCU5T4K=sLMF0t0vrn_J=nhASgrtGH7%2BqbeBrZoXr1MmQ@mail.gmail.com> <513F571A.5070007@rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Mar 12, 2013 at 5:26 PM, Alan Cox <alc@rice.edu> wrote: > On 03/12/2013 07:04, Attilio Rao wrote: >> On Tue, Mar 12, 2013 at 7:14 AM, Alan Cox <alc@freebsd.org> wrote: >>> Author: alc >>> Date: Tue Mar 12 06:14:31 2013 >>> New Revision: 248185 >>> URL: http://svnweb.freebsd.org/changeset/base/248185 >>> >>> Log: >>> When transferring the page from one object to another, don't insert the >>> page into its new object until the page's pindex has been updated. >>> Otherwise, one code path within vm_radix_insert() may use the wrong >>> pindex value. >> This is just a style change really because the code already subtracts >> offindxstart from current m->pindex when inserting. The pindex on the >> page is then adjusted when the real subtraction happens. >> IIRC, I did this way because of less diffs against -CURRENT, but the >> code looks correct to me already in the older version. >> > > No, as I say in the commit message, there is a path in vm_radix_insert() > that uses "page->pindex" and not the "index" argument: > > /* > * A new node is needed because the right insertion level is > reached. > * Setup the new intermediate node and add the 2 children: the > * new object and the older edge. > */ > tmp2 = vm_radix_node_get(vm_radix_trimkey(page->pindex, clev - > 1), 2, > clev); > > Prior to this change, if we follow this path, then we are using the > wrong "pindex" value at this point in vm_radix_insert(). My bad, that should use "index" as it was supposed to be, not the page one. I likely added it as a cleanup after having reworked all the other functions, introducing the bug. It is courious it never showed up. I'd rather fix vm_radix_insert() to not consider the page index. This is a step forward anyway in generalizing the code. Attilio -- Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndDvoub2mXRj-orjwS2dH_qfhKPSBE7q5hbNJAtKbQJM-g>