Subject: Re: [PATCH] Use extents for recording what swap is allocated.
From: Nigel Cunningham <ncunningham@linuxmail.org>
To: Pavel Machek <pavel@ucw.cz>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>, Andrew Morton <akpm@osdl.org>,
       LKML <linux-kernel@vger.kernel.org>
In-Reply-To: <20061024213402.GC5662@elf.ucw.cz>
References: <1161576857.3466.9.camel@nigel.suspend2.net>
	 <200610242208.34426.rjw@sisk.pl>  <20061024213402.GC5662@elf.ucw.cz>
Content-Type: text/plain
Date: Wed, 25 Oct 2006 08:15:53 +1000
Message-Id: <1161728153.22729.22.camel@nigel.suspend2.net>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1920
Lines: 70

Hi.

On Tue, 2006-10-24 at 23:34 +0200, Pavel Machek wrote:
> Hi!
> 
> > > Switch from bitmaps to using extents to record what swap is allocated;
> > > they make more efficient use of memory, particularly where the allocated
> > > storage is small and the swap space is large.
> > 
> > As I said before, I like the overall idea, but I have a bunch of
> > comments.
> 
> Okay, if Rafael likes it... lets take a look.
> 
> First... what is the _worst case_ overhead? AFAICT extents are very
> good at the best case, but tend to suck for the worst case...?

That's right. In using this, we're relying on the fact that the swap
allocator tries to act sensibly. I've only seen worse case performance
when a user had two swap devices with the same priority (striped), but
that was a bug. :)

> > > +#include <linux/suspend.h>
> > > +#include "extent.h"
> > > +
> > > +/* suspend_get_extent
> > > + *
> > > + * Returns a free extent. May fail, returning NULL instead.
> > > + */
> 
> Your comments are nice, and quite close to linuxdoc... Can we make
> them proper linuxdoc?

Ok.

> > > +/* suspend_put_extent_chain.
> > > + *
> > > + * Frees a whole chain of extents.
> > > + */
> > > +void suspend_put_extent_chain(struct extent_chain *chain)
> > 
> > I'd call it suspend_free_all_extents().
> 
> This is actually important. As it does undocditional free(), it may
> not be called "put".

Ok.

> > > +#ifndef EXTENT_H
> > > +#define EXTENT_H
> > > +
> > > +struct extent {
> > > +	unsigned long minimum, maximum;
> > 
> > Well, I'd use shorter names, but whatever.
> 
> Actually, minimum and 
> 	  maximum look too similar. start/end are really better names.

Ok.

Thanks!

Nigel

-
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/