On Thu, Mar 03, 2005 at 01:57:49PM -0800, Luck, Tony wrote: > >Here are the changes again. I have not received a response from you > >about the restructered shrink routine. I have incorporated David > >Mosberger's recommendations. > > Your new shrink routine looks ok, but you didn't address the > double role played by NODE_FREE_PAGES_SHIFT ... you just dropped the > comment that explained one of its uses - which isn't what I'd hoped > for :-)
The comment was left from the earlier set of patches where it had a single role. In that patch, we picked a number of pages to free, freed that many, and went on with life expecting the next pass to free a group. Since the changes gave it a more complex role and the code was not that complex, I figured I would remove the comment and let anybody planning on changing it in the future read the code to figure out the two roles. > > I'm also a bit uncomfortable with: > > + preempt_enable(); > + preempt_disable(); > > For a kernel with CONFIG_PREEMPT=n, this is a no-op ... so if there > is a ton of extra pages on the quicklist, we'll loop freeing 16 at > a time and re-computing how many to free, with no pause to take a > breath (or a clock tick). What do you want me to do. I don't see anywhere else in the kernel that these two lines are directly adjacent. Most places that do the disable/enable are in a function which does one thing. That is occasionally contained inside a larger loop. We can not do that since part of our outer loop control is based on the per_cpu variable we are expecting to not change. I suppose I could elminitate the disable/enable entirely. I haven't thought all the way through the possibilities, but I would guess we could free a couple extra pages, but who cares. You tell me what to do. Robin - To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
