On Sat, 23 Jun 2007, Peter Zijlstra wrote:

quoted text
> On Thu, 2007-06-21 at 16:08 -0700, Linus Torvalds wrote:
> > 
> > The vm_dirty_ratio thing is a global value, and I think we need that 
> > regardless (for the independent issue of memory deadlocks etc), but if we 
> > *additionally* had a per-device throttle that was based on some kind of 
> > adaptive thing, we could probably raise the global (hard) vm_dirty_ratio a 
> > lot.
> 
> I just did quite a bit of that:
> 
>   http://lkml.org/lkml/2007/6/14/437

Ok, that does look interesting.

A few comments:

 - Cosmetic: please please *please* don't do this:

	-	if (atomic_long_dec_return(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH) {
	+	if (atomic_long_dec_return(&nfss->writeback) <
	+			NFS_CONGESTION_OFF_THRESH)

   we had a discussion about this not that long ago, and it drives me wild 
   to see people split lines like that. It used to be readable. Now it's 
   not.

   If it's the checkpatch.pl thing that caused you to split it like that, 
   I think we should just change the value where we start complaining. 
   Maybe set it at 95 characters per line or something.

 - I appreciate the extensive comments on floating proportions, and the 
   code looks really quite clean (apart from the cosmetic thing about) 
   from a quick look-through.

   HOWEVER. It does seem to be a bit of an overkill. Do we really need to 
   be quite that clever, and do we really need to do 64-bit calculations 
   for this? The 64-bit ops in particular seem quite iffy: if we ever 
   actually pass in an amount that doesn't fit in 32 bits, we'll turn 
   those per-cpu counters into totally synchronous global counters, which 
   seems to defeat the whole point of them. So I'm a bit taken aback by 
   that whole "mod64" thing

   (I also hate the naming. I don't think "..._mod()" was a good name to 
   begin with: "mod" means "modulus" to me, not "modify". Making it be 
   called "mod64" just makes it even *worse*, since it's now _obviously_ 
   about modulus - but isn't)

So I'd appreciate some more explanations, but I'd also appreciate some 
renaming of those functions. What used to be pretty bad naming just turned 
*really* bad, imnsho.

		Linus
-
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/