[PATCH] expand_resource

[PATCH] expand_resource

[Matthew Wilcox]

Rewrite expand_resource() to be racefree and move it to kernel/resource.c.

Index: linux-2.6/kernel/resource.c
===================================================================
RCS file: /var/cvs/linux-2.6/kernel/resource.c,v
retrieving revision 1.3.2.1
retrieving revision 1.4
diff -u -p -r1.3.2.1 -r1.4
--- linux-2.6/kernel/resource.c	28 Sep 2003 02:27:48 -0000	1.3.2.1
+++ linux-2.6/kernel/resource.c	28 Sep 2003 04:06:24 -0000	1.4
@@ -266,6 +268,53 @@ int allocate_resource(struct resource *r
 		err = -EBUSY;
 	write_unlock(&resource_lock);
 	return err;
+}
+
+/*
+ * Expand an existing resource by size amount.
+ */
+int expand_resource(struct resource *res, unsigned long size,
+			   unsigned long align)
+{
+	unsigned long start, end;
+
+	/* see if we can expand above */
+	end = (res->end + size + align - 1) & ~(align - 1);
+
+	write_lock(&resource_lock);
+	if (res->sibling) {
+		if (res->sibling->start > end)
+			goto end;
+	} else {
+		if (res->parent->end >= end)
+			goto end;
+	}
+
+	/* now try below */
+	start = ((res->start - size + align) & ~(align - 1)) - align;
+
+	if (res->parent->child == res) {
+		if (res->start <= start)
+			goto start;
+	} else {
+		struct resource *prev = res->parent->child;
+		while (prev->sibling != res)
+			prev = prev->sibling;
+		if (prev->end < start)
+			goto start;
+	}
+
+	write_unlock(&resource_lock);
+	return -ENOMEM;
+
+ start:
+	res->start = start;
+	write_unlock(&resource_lock);
+	return 0;
+ end:
+	res->end = end;
+	write_unlock(&resource_lock);
+	return 0;
 }
 
 /*
Index: linux-2.6/drivers/parisc/ccio-dma.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/parisc/ccio-dma.c,v
retrieving revision 1.7.2.1
retrieving revision 1.7
diff -u -p -r1.7.2.1 -r1.7
--- linux-2.6/drivers/parisc/ccio-dma.c	28 Sep 2003 02:27:09 -0000	1.7.2.1
+++ linux-2.6/drivers/parisc/ccio-dma.c	27 Sep 2003 20:03:16 -0000	1.7
@@ -1534,42 +1534,6 @@ static void __init ccio_init_resources(s
 			(unsigned long)&ioc->ioc_hpa->io_io_low_hv);
 }
 
-static int expand_resource(struct resource *res, unsigned long size,
-			   unsigned long align)
-{
-	struct resource *temp_res;
-	unsigned long start = res->start;
-	unsigned long end ;
-
-	/* see if we can expand above */
-	end = (res->end + size + align - 1) & ~(align - 1);;
-	
-	temp_res = __request_region(res->parent, res->end, end - res->end,
-				    "expansion");
-	if(!temp_res) {
-		/* now try below */
-		start = ((res->start - size + align) & ~(align - 1)) - align;
-		end = res->end;
-		temp_res = __request_region(res->parent, start, size,
-					    "expansion");	
-		if(!temp_res) {
-			return -ENOMEM;
-		}
-	} 
-	release_resource(temp_res);
-	temp_res = res->parent;
-	release_resource(res);
-	res->start = start;
-	res->end = end;
-
-	/* This could be caused by some sort of race.  Basically, if
-	 * this tripped something stole the region we just reserved
-	 * and then released to check for expansion */
-	BUG_ON(request_resource(temp_res, res) != 0);
-
-	return 0;
-}
-
 static void expand_ioc_area(struct resource *parent, struct ioc *ioc,
 			    unsigned long size,	unsigned long min,
 			    unsigned long max, unsigned long align)
Index: linux-2.6/include/linux/ioport.h
===================================================================
RCS file: /var/cvs/linux-2.6/include/linux/ioport.h,v
retrieving revision 1.2.2.1
retrieving revision 1.2
diff -u -p -r1.2.2.1 -r1.2
--- linux-2.6/include/linux/ioport.h	28 Sep 2003 02:27:45 -0000	1.2.2.1
+++ linux-2.6/include/linux/ioport.h	27 Sep 2003 20:03:18 -0000	1.2
@@ -97,6 +97,8 @@ extern int allocate_resource(struct reso
 			     void (*alignf)(void *, struct resource *,
 					    unsigned long, unsigned long),
 			     void *alignf_data);
+int expand_resource(struct resource *res, unsigned long size,
+		    unsigned long align);
 
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name))

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] expand_resource

[Russell King]

On Tue, Sep 30, 2003 at 10:04:10PM +0100, Matthew Wilcox wrote:
> Rewrite expand_resource() to be racefree and move it to kernel/resource.c.
> 
>...
> +/*
> + * Expand an existing resource by size amount.
> + */
> +int expand_resource(struct resource *res, unsigned long size,
> +			   unsigned long align)
> +{

Could we have the kerneldoc stuff added to this function?  My main
question though is - what is the intended purpose of "align" ?  As
it is implemented, it seems to have a weird behaviour:

- if we expand the "end" of the resource, we round it towards an
  address which is a multiple of align, but "start" may not be
  a multiple of align.
- if we expand "start", we round it down towards a multiple of
  align.  However, "end" may not be a multiple of align.

This may actually be of use to PCMCIA IO space handling - we only
have two windows, but we may need to expand them if we have a multi-
function card if we have more than 2 areas to map.  In this case,
we'd need to know whether "start" was expanded or "end" was expanded
since we can't change the use of the already-allocated resource.

It may make sense to do something more generic, like:

int adjust_resource(struct resource *res, unsigned long start,
		    unsigned long end);

so that the caller knows what he's requesting and knows whether that
change succeeded or failed.  However, it is something I'd need to
look deeper into when I have more time available to look at such
stuff, so please don't take the above as a well thought-out solution.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: [PATCH] expand_resource

[Matthew Wilcox]

On Tue, Sep 30, 2003 at 10:27:08PM +0100, Russell King wrote:
> On Tue, Sep 30, 2003 at 10:04:10PM +0100, Matthew Wilcox wrote:
> > +/*
> > + * Expand an existing resource by size amount.
> > + */
> > +int expand_resource(struct resource *res, unsigned long size,
> > +			   unsigned long align)
> > +{
> 
> Could we have the kerneldoc stuff added to this function?

Certainly.  Rather than answering your question below directly,
let's see if the kerneldoc answers it (and if not, then the kerneldoc
needs to be made clearer).

/**
 * expand_resource - Expand an existing resource
 * @res: Resource already linked into the resource tree.
 * @size: Amount of additional space requested below @res.
 * @align: Required alignment of new space.
 *
 * This function is intended to be called when we need more space
 * below an existing resource than currently exists.  The @size and
 * @align parameters describe the requirements of the child resource,
 * not the parent.  Note that this call does not attempt to expand
 * the parent of @res as that might require reprogramming devices.
 * If a caller would like this behaviour, they must handle the recursion.
 */

> My main
> question though is - what is the intended purpose of "align" ?  As
> it is implemented, it seems to have a weird behaviour:
> 
> - if we expand the "end" of the resource, we round it towards an
>   address which is a multiple of align, but "start" may not be
>   a multiple of align.
> - if we expand "start", we round it down towards a multiple of
>   align.  However, "end" may not be a multiple of align.
> 
> This may actually be of use to PCMCIA IO space handling - we only
> have two windows, but we may need to expand them if we have a multi-
> function card if we have more than 2 areas to map.  In this case,
> we'd need to know whether "start" was expanded or "end" was expanded
> since we can't change the use of the already-allocated resource.

Heh, this sounds almost identical to the PARISC problem.  I knew other
users would come out of the woodwork if I sent the patch ;-)  We also
need to know whether the start or end was changed to reprogram the
parent device -- see the caller in drivers/parisc/ccio-dma.c:

        if (expand_resource(parent, size, align) != 0) {
                printk(KERN_ERR "Unable to expand %s window by 0x%lx\n",
                       parent->name, size);
                return;
        }
        __raw_writel(((parent->start)>>16) | 0xffff0000,
                     (unsigned long)&(ioc->ioc_hpa->io_io_low));
        __raw_writel(((parent->end)>>16) | 0xffff0000,
                     (unsigned long)&(ioc->ioc_hpa->io_io_high));

> It may make sense to do something more generic, like:
> 
> int adjust_resource(struct resource *res, unsigned long start,
> 		    unsigned long end);
> 
> so that the caller knows what he's requesting and knows whether that
> change succeeded or failed.  However, it is something I'd need to
> look deeper into when I have more time available to look at such
> stuff, so please don't take the above as a well thought-out solution.

I don't like that.  It puts more work into the caller, but doesn't
seem to simplify the _resource function.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] expand_resource

[Russell King]

On Tue, Sep 30, 2003 at 11:14:11PM +0100, Matthew Wilcox wrote:
> Certainly.  Rather than answering your question below directly,
> let's see if the kerneldoc answers it (and if not, then the kerneldoc
> needs to be made clearer).
> 
> /**
>  * expand_resource - Expand an existing resource
>  * @res: Resource already linked into the resource tree.
>  * @size: Amount of additional space requested below @res.
>  * @align: Required alignment of new space.
>  *
>  * This function is intended to be called when we need more space
>  * below an existing resource than currently exists.  The @size and
>  * @align parameters describe the requirements of the child resource,
>  * not the parent.  Note that this call does not attempt to expand
>  * the parent of @res as that might require reprogramming devices.
>  * If a caller would like this behaviour, they must handle the recursion.
>  */
> 
> > My main
> > question though is - what is the intended purpose of "align" ?  As
> > it is implemented, it seems to have a weird behaviour:
> > 
> > - if we expand the "end" of the resource, we round it towards an
> >   address which is a multiple of align, but "start" may not be
> >   a multiple of align.
> > - if we expand "start", we round it down towards a multiple of
> >   align.  However, "end" may not be a multiple of align.

Ok, your description covers the latter case, and seems quite reasonable
given the arguments.  The former case seems to be a little less clear
though.

> Heh, this sounds almost identical to the PARISC problem.  I knew other
> users would come out of the woodwork if I sent the patch ;-)  We also
> need to know whether the start or end was changed to reprogram the
> parent device -- see the caller in drivers/parisc/ccio-dma.c:
> 
> > It may make sense to do something more generic, like:
> > 
> > int adjust_resource(struct resource *res, unsigned long start,
> > 		    unsigned long end);
> > 
> > so that the caller knows what he's requesting and knows whether that
> > change succeeded or failed.  However, it is something I'd need to
> > look deeper into when I have more time available to look at such
> > stuff, so please don't take the above as a well thought-out solution.
> 
> I don't like that.  It puts more work into the caller, but doesn't
> seem to simplify the _resource function.

The problem with expand_resource as it currently stands is that it can
expand either at the start or the end of the resource, and we don't know
which it is going to do.  PCMCIA is one example where this information
matters - not only do we need to know the right IO range for each IO
window, but we also need to know the IO address for each device.

This means that if we were to expand these window resources, we must
know where the new space was allocated.

I think we can both agree that saving the old resources start, end
and then comparing it after expand_resource() is fairly disgusting.

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Re: [PATCH] expand_resource

[Matthew Wilcox]

On Tue, Sep 30, 2003 at 11:33:52PM +0100, Russell King wrote:
> On Tue, Sep 30, 2003 at 11:14:11PM +0100, Matthew Wilcox wrote:
> > /**
> >  * expand_resource - Expand an existing resource
> >  * @res: Resource already linked into the resource tree.
> >  * @size: Amount of additional space requested below @res.
> >  * @align: Required alignment of new space.
> >  *
> >  * This function is intended to be called when we need more space
> >  * below an existing resource than currently exists.  The @size and
> >  * @align parameters describe the requirements of the child resource,
> >  * not the parent.  Note that this call does not attempt to expand
> >  * the parent of @res as that might require reprogramming devices.
> >  * If a caller would like this behaviour, they must handle the recursion.
> >  */
> > 
> > > My main
> > > question though is - what is the intended purpose of "align" ?  As
> > > it is implemented, it seems to have a weird behaviour:
> > > 
> > > - if we expand the "end" of the resource, we round it towards an
> > >   address which is a multiple of align, but "start" may not be
> > >   a multiple of align.
> > > - if we expand "start", we round it down towards a multiple of
> > >   align.  However, "end" may not be a multiple of align.
> 
> Ok, your description covers the latter case, and seems quite reasonable
> given the arguments.  The former case seems to be a little less clear
> though.

I don't understand what you mean here ... Expanding start is clear,
but expanding end isn't clear?

> The problem with expand_resource as it currently stands is that it can
> expand either at the start or the end of the resource, and we don't know
> which it is going to do.  PCMCIA is one example where this information
> matters - not only do we need to know the right IO range for each IO
> window, but we also need to know the IO address for each device.
> 
> This means that if we were to expand these window resources, we must
> know where the new space was allocated.
> 
> I think we can both agree that saving the old resources start, end
> and then comparing it after expand_resource() is fairly disgusting.

Certainly.  For CCIO, we just program both high and low registers.
I guess we could do something like returning the value that changed
and in the caller:

	int new = expand_resource(res, size, align);

	if (new < 0) {
		return new;
	} else if (new == res->start) {
		/* program start */
	} else {
		/* program end */
	}

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [PATCH] expand_resource

[Matthias Urlichs]

Hi, Matthew Wilcox wrote:
> 	int new = expand_resource(res, size, align);
> 
> 	if (new < 0) {
> 		return new;
> 	} else if (new == res->start) {
> 		/* program start */
> 	} else {
> 		/* program end */
> 	}

Umm, these /*program*/ starts/ends look backwards. 

Personally, I'd check both independently. What stops expand_resource
from doing both if there's no increase possible in only one direction to
make room for <size>?

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Battle, n.:
	A method of untying with the teeth a political knot that
	will not yield to the tongue.
		-- Ambrose Bierce