diff mbox

[04/15] ps3vram: Replace mutex by spinlock + list

Message ID 1241791284-11490-5-git-send-email-Geert.Uytterhoeven@sonycom.com
State New
Headers show

Commit Message

Geert Uytterhoeven May 8, 2009, 2:01 p.m. UTC 
Remove the mutex serializing access to the cache.
Instead, queue up new requests on a list if the driver is busy.

This improves sequential write performance by ca. 2%.

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: Jim Paris <jim@jtan.com>
---
 drivers/block/ps3vram.c |   55 ++++++++++++++++++++++++++++++++++------------
 1 files changed, 40 insertions(+), 15 deletions(-)

Comments

Andrew Morton May 8, 2009, 11:05 p.m. UTC | #1 
On Fri,  8 May 2009 16:01:13 +0200
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> +static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
> +{
> +	struct ps3_system_bus_device *dev = q->queuedata;
> +	struct ps3vram_priv *priv = dev->core.driver_data;
> +
> +	dev_dbg(&dev->core, "%s\n", __func__);
> +
> +	spin_lock_irq(&priv->lock);
> +	if (priv->tail) {
> +		priv->tail->bi_next = bio;
> +		priv->tail = bio;
> +		spin_unlock_irq(&priv->lock);
> +		return 0;
> +	}
> +
> +	priv->tail = bio;
> +	spin_unlock_irq(&priv->lock);
> +
> +	do {
> +		bio = ps3vram_do_bio(dev, bio);
> +	} while (bio);

Is there something which prevents two threads of control from walking
the same list at the same time?

> +
>  	return 0;
>  }
Christoph Hellwig May 10, 2009, 7:05 p.m. UTC | #2 
On Fri, May 08, 2009 at 04:01:13PM +0200, Geert Uytterhoeven wrote:
> Remove the mutex serializing access to the cache.
> Instead, queue up new requests on a list if the driver is busy.

This should use the bio list helpers moved into bio.h in commit
8f3d8ba20e67991b531e9c0227dcd1f99271a32c
Geert Uytterhoeven May 11, 2009, 6:38 a.m. UTC | #3 
On Fri, 8 May 2009, Andrew Morton wrote:
> On Fri,  8 May 2009 16:01:13 +0200
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > +static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
> > +{
> > +	struct ps3_system_bus_device *dev = q->queuedata;
> > +	struct ps3vram_priv *priv = dev->core.driver_data;
> > +
> > +	dev_dbg(&dev->core, "%s\n", __func__);
> > +
> > +	spin_lock_irq(&priv->lock);
> > +	if (priv->tail) {
> > +		priv->tail->bi_next = bio;
> > +		priv->tail = bio;
> > +		spin_unlock_irq(&priv->lock);
> > +		return 0;
                ^^^^^^^^
> > +	}
> > +
> > +	priv->tail = bio;
> > +	spin_unlock_irq(&priv->lock);
> > +
> > +	do {
> > +		bio = ps3vram_do_bio(dev, bio);
> > +	} while (bio);
> 
> Is there something which prevents two threads of control from walking
> the same list at the same time?

Sure: only the thread who adds the first request to the empty list will walk
the list later.

With kind regards,

Geert Uytterhoeven
Software Architect
Techsoft Centre

Technology and Software Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
diff mbox

Patch

diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
index 3c9ad19..1396038 100644
--- a/drivers/block/ps3vram.c
+++ b/drivers/block/ps3vram.c
@@ -81,8 +81,8 @@  struct ps3vram_priv {
 
 	struct ps3vram_cache cache;
 
-	/* Used to serialize cache/DMA operations */
-	struct mutex lock;
+	spinlock_t lock;	/* protecting list of bios */
+	struct bio *tail;
 };
 
 
@@ -449,8 +449,6 @@  static int ps3vram_read(struct ps3_system_bus_device *dev, loff_t from,
 		offset = (unsigned int) (from & (priv->cache.page_size - 1));
 		avail  = priv->cache.page_size - offset;
 
-		mutex_lock(&priv->lock);
-
 		entry = ps3vram_cache_match(dev, from);
 		cached = CACHE_OFFSET + entry * priv->cache.page_size + offset;
 
@@ -462,8 +460,6 @@  static int ps3vram_read(struct ps3_system_bus_device *dev, loff_t from,
 			avail = count;
 		memcpy(buf, priv->xdr_buf + cached, avail);
 
-		mutex_unlock(&priv->lock);
-
 		buf += avail;
 		count -= avail;
 		from += avail;
@@ -494,8 +490,6 @@  static int ps3vram_write(struct ps3_system_bus_device *dev, loff_t to,
 		offset = (unsigned int) (to & (priv->cache.page_size - 1));
 		avail  = priv->cache.page_size - offset;
 
-		mutex_lock(&priv->lock);
-
 		entry = ps3vram_cache_match(dev, to);
 		cached = CACHE_OFFSET + entry * priv->cache.page_size + offset;
 
@@ -509,8 +503,6 @@  static int ps3vram_write(struct ps3_system_bus_device *dev, loff_t to,
 
 		priv->cache.tags[entry].flags |= CACHE_PAGE_DIRTY;
 
-		mutex_unlock(&priv->lock);
-
 		buf += avail;
 		count -= avail;
 		to += avail;
@@ -552,17 +544,17 @@  static void __devinit ps3vram_proc_init(struct ps3_system_bus_device *dev)
 		dev_warn(&dev->core, "failed to create /proc entry\n");
 }
 
-static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
+static struct bio *ps3vram_do_bio(struct ps3_system_bus_device *dev,
+				  struct bio *bio)
 {
-	struct ps3_system_bus_device *dev = q->queuedata;
+	struct ps3vram_priv *priv = dev->core.driver_data;
 	int write = bio_data_dir(bio) == WRITE;
 	const char *op = write ? "write" : "read";
 	loff_t offset = bio->bi_sector << 9;
 	int error = 0;
 	struct bio_vec *bvec;
 	unsigned int i;
-
-	dev_dbg(&dev->core, "%s\n", __func__);
+	struct bio *next;
 
 	bio_for_each_segment(bvec, bio, i) {
 		/* PS3 is ppc64, so we don't handle highmem */
@@ -593,7 +585,40 @@  static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
 	dev_dbg(&dev->core, "%s completed\n", op);
 
 out:
+	spin_lock_irq(&priv->lock);
+	next = bio->bi_next;
+	if (!next)
+		priv->tail = NULL;
+	else
+		bio->bi_next = NULL;
+	spin_unlock_irq(&priv->lock);
+
 	bio_endio(bio, error);
+	return next;
+}
+
+static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
+{
+	struct ps3_system_bus_device *dev = q->queuedata;
+	struct ps3vram_priv *priv = dev->core.driver_data;
+
+	dev_dbg(&dev->core, "%s\n", __func__);
+
+	spin_lock_irq(&priv->lock);
+	if (priv->tail) {
+		priv->tail->bi_next = bio;
+		priv->tail = bio;
+		spin_unlock_irq(&priv->lock);
+		return 0;
+	}
+
+	priv->tail = bio;
+	spin_unlock_irq(&priv->lock);
+
+	do {
+		bio = ps3vram_do_bio(dev, bio);
+	} while (bio);
+
 	return 0;
 }
 
@@ -613,7 +638,7 @@  static int __devinit ps3vram_probe(struct ps3_system_bus_device *dev)
 		goto fail;
 	}
 
-	mutex_init(&priv->lock);
+	spin_lock_init(&priv->lock);
 	dev->core.driver_data = priv;
 
 	priv = dev->core.driver_data;