proposed consolidate spin_lock/unlock waiting with spin_unlock_wait

PATCH In review Nicholas Mc Guire 2 participants 6 messages Dec 2, 2013 – Dec 4, 2013

Subsystems (6)

Network drivers Arm (32-bit) Arm sub-architectures Staging +2 more subsystems

lore

Share & tools

lore

Nicholas Mc Guire

author

2013-12-02 14:56 UTC #0 lore raw

PATCH 1/1 proposed consolidate spin_lock/unlock waiting with spin_unlock_wait

From 5b3d0b88438846cbdd888798c2fb04e20c69e95d Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <[email protected]>
Date: Sun, 1 Dec 2013 22:53:18 -0500
Subject: [PATCH] consolidate spin_lock/unlock waiting with spin_unlock_wait

Hi !

Not sure if this is the right place to put this... This patch proposes a small API consolidation.

in the following files there is a spin_lock/unlock in use for waiting on some possibly concurrent thread - that is there is a

	spin_lock(lock);
	spin_unlock(lock);

so in the contended case the calling thread will wait for any concurrent access.

arch/blackfin/mach-bf561/smp.c lock/unlock at lines 72 73 arch/arm/mach-sti/platsmp.c lock/unlock at lines 53 54 arch/cris/arch-v32/drivers/mach-fs/gpio.c lock/unlock at lines 764 765 drivers/staging/lustre/lustre/obdclass/llog_obd.c lock/unlock at lines 87 89 drivers/net/ethernet/chelsio/cxgb/cxgb2.c lock/unlock at lines 287 288 drivers/net/ethernet/ti/cpmac.c lock/unlock at lines 582 583 fs/fscache/object.c lock/unlock at lines 692 693

in
commit c2f21ce2e31286a0a32f8da0a7856e9ca1122ef3
Author: Thomas Gleixner <[email protected]>
Date: Wed Dec 2 20:02:59 2009 +0100

locking: Implement new raw_spinlock

the api was extended with include/linux/spinlock.h:spin_unlock_wait() for just this purpose (I think atleast) so the below patch changes these currently hard-coded spin_lock/unlock to use the available API. so as its in spinlock.h and the code in question is using spin_lock/unlock no new header file inclusion is needed.

all the above listed cases are consecutive spin_lock/spin_unlock. the one-line offset in llog_obd.c at lines 87 89 is due to a comment not code so it also is a consecutive spin_lock/spin_unlock,

If these changes make any sense should this go out to all of the code authors for review/ack or is this simple enough for it to just go out via kernel-janitors/LKML for review ?

patch is against 3.13.0-rc1 (linux-stable Greg KH tree)

Signed-off-by Nicholas Mc Guire <[email protected]>

---
 arch/arm/mach-sti/platsmp.c                       |    3 +--
 arch/blackfin/mach-bf561/smp.c                    |    3 +--
 arch/cris/arch-v32/drivers/mach-fs/gpio.c         |    3 +--
 drivers/net/ethernet/chelsio/cxgb/cxgb2.c         |    3 +--
 drivers/net/ethernet/ti/cpmac.c                   |    3 +--
 drivers/staging/lustre/lustre/obdclass/llog_obd.c |    3 +--
 fs/fscache/object.c                               |    3 +--
 7 files changed, 7 insertions(+), 14 deletions(-)

arch/arm/mach-sti/platsmp.c +1 -2

index dce50d9..e05b29e 100644
@@ -50,8 +50,7 @@ void sti_secondary_init(unsigned int cpu)
 	/*
 	 * Synchronise with the boot thread.
 	 */
-	spin_lock(&boot_lock);
-	spin_unlock(&boot_lock);
+	spin_unlock_wait(&boot_lock);
 }
 
 int sti_boot_secondary(unsigned int cpu, struct task_struct *idle)

arch/blackfin/mach-bf561/smp.c +1 -2

index 11789be..463d839 100644
@@ -69,8 +69,7 @@ void platform_secondary_init(unsigned int cpu)
 	SSYNC();
 
 	/* We are done with local CPU inits, unblock the boot CPU. */
-	spin_lock(&boot_lock);
-	spin_unlock(&boot_lock);
+	spin_unlock_wait(&boot_lock);
 }
 
 int platform_boot_secondary(unsigned int cpu, struct task_struct *idle)

arch/cris/arch-v32/drivers/mach-fs/gpio.c +1 -2

index a2ac091..dec2e43 100644
@@ -761,8 +761,7 @@ virtual_gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		/* Clear alarm for bits with 1 in arg. */
 		priv->highalarm &= ~arg;
 		priv->lowalarm  &= ~arg;
-		spin_lock(&alarm_lock);
-		spin_unlock(&alarm_lock);
+		spin_unlock_wait(&alarm_lock);
 		break;
 	case IO_CFG_WRITE_MODE:
 	{

drivers/net/ethernet/chelsio/cxgb/cxgb2.c +1 -2

index 1d02105..1aef5f2 100644
@@ -284,8 +284,7 @@ static int cxgb_close(struct net_device *dev)
 	    !(adapter->open_device_map & PORT_MASK)) {
 		/* Stop statistics accumulation. */
 		smp_mb__after_clear_bit();
-		spin_lock(&adapter->work_lock);   /* sync with update task */
-		spin_unlock(&adapter->work_lock);
+		spin_unlock_wait(&adapter->work_lock);
 		cancel_mac_stats_update(adapter);
 	}

drivers/net/ethernet/ti/cpmac.c +1 -2

index 2dc16b6..e62f812 100644
@@ -579,8 +579,7 @@ static int cpmac_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
-	spin_lock(&priv->lock);
-	spin_unlock(&priv->lock);
+	spin_unlock_wait(&priv->lock);
 	desc->dataflags = CPMAC_SOP | CPMAC_EOP | CPMAC_OWN;
 	desc->skb = skb;
 	desc->data_mapping = dma_map_single(&dev->dev, skb->data, len,

drivers/staging/lustre/lustre/obdclass/llog_obd.c +1 -2

index 71817af..e6c0b90 100644
@@ -84,9 +84,8 @@ int __llog_ctxt_put(const struct lu_env *env, struct llog_ctxt *ctxt)
 	spin_unlock(&olg->olg_lock);
 
 	obd = ctxt->loc_obd;
-	spin_lock(&obd->obd_dev_lock);
 	/* sync with llog ctxt user thread */
-	spin_unlock(&obd->obd_dev_lock);
+	spin_unlock_wait(&obd->obd_dev_lock);
 
 	/* obd->obd_starting is needed for the case of cleanup
 	 * in error case while obd is starting up. */

fs/fscache/object.c +1 -2

index 53d35c5..43e1331 100644
@@ -692,8 +692,7 @@ static const struct fscache_state *fscache_drop_object(struct fscache_object *ob
 	/* Prevent a race with our last child, which has to signal EV_CLEARED
 	 * before dropping our spinlock.
 	 */
-	spin_lock(&object->lock);
-	spin_unlock(&object->lock);
+	spin_unlock_wait(&object->lock);
 
 	/* Discard from the cache's collection of objects */
 	spin_lock(&cache->object_list_lock);

Signature · 4 lines

1.7.2.5


----- End forwarded message -----

Dan Carpenter

2013-12-02 15:45 UTC #1 lore raw

I think the patch is generally ok. It needs to be split up and sent through the individual maintainers. The changelog has a lot of extra verbiage.

Quoted 5 lines · Nicholas Mc Guire

Quoted 1 line
From 5b3d0b88438846cbdd888798c2fb04e20c69e95d Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <[email protected]>
Date: Sun, 1 Dec 2013 22:53:18 -0500
Subject: [PATCH] consolidate spin_lock/unlock waiting with spin_unlock_wait

Don't include these bits in the patch and don't indent the changelog by one space (don't indent at all, it should be hard left).

Quoted 14 lines

Hi !
Not sure if this is the right place to put this... This patch proposes a small API consolidation.
in the following files there is a spin_lock/unlock in use for waiting on some possibly concurrent thread - that is there is a
	spin_lock(lock);
	spin_unlock(lock);
so in the contended case the calling thread will wait for any concurrent access.

Quoted 7 lines

arch/blackfin/mach-bf561/smp.c lock/unlock at lines 72 73 arch/arm/mach-sti/platsmp.c lock/unlock at lines 53 54 arch/cris/arch-v32/drivers/mach-fs/gpio.c lock/unlock at lines 764 765 drivers/staging/lustre/lustre/obdclass/llog_obd.c lock/unlock at lines 87 89 drivers/net/ethernet/chelsio/cxgb/cxgb2.c lock/unlock at lines 287 288 drivers/net/ethernet/ti/cpmac.c lock/unlock at lines 582 583 fs/fscache/object.c lock/unlock at lines 692 693

This block is not useful.

Quoted 8 lines

in
commit c2f21ce2e31286a0a32f8da0a7856e9ca1122ef3
Author: Thomas Gleixner <[email protected]>
Date: Wed Dec 2 20:02:59 2009 +0100
locking: Implement new raw_spinlock

This is not the patch which introduced spin_unlock_wait(). That function was around before we adopted git.

Quoted 5 lines

the api was extended with include/linux/spinlock.h:spin_unlock_wait() for just this purpose (I think atleast) so the below patch changes these currently hard-coded spin_lock/unlock to use the available API. so as its in spinlock.h and the code in question is using spin_lock/unlock no new header file inclusion is needed.

We hopefully can assume you compile tested these so this information about header files is redundant. If you didn't compile test, then we will get really annoyed.

Quoted 4 lines

all the above listed cases are consecutive spin_lock/spin_unlock. the one-line offset in llog_obd.c at lines 87 89 is due to a comment not code so it also is a consecutive spin_lock/spin_unlock,

Obvious. Don't include this in the commit message.

Quoted 7 lines

If these changes make any sense should this go out to all of the code authors for review/ack or is this simple enough for it to just go out via kernel-janitors/LKML for review ?
patch is against 3.13.0-rc1 (linux-stable Greg KH tree)

Don't do patches against stable. Do them against the current -rc or against linux-next. In theory, the current -rc is better because then you can test them without other patches affecting the results, but in real life, you don't have the hardware for most of these and I doubt you are going to test the filesystem ones.

drivers/net/ethernet/chelsio/cxgb/cxgb2.c static int cxgb_close(struct net_device *dev) · line 287

Quoted 5 lines

drivers/net/ethernet/chelsio/cxgb/cxgb2.c +0 -1

@@ -284,8 +284,7 @@ static int cxgb_close(struct net_device *dev)
 	    !(adapter->open_device_map & PORT_MASK)) {
 		/* Stop statistics accumulation. */
 		smp_mb__after_clear_bit();
-		spin_lock(&adapter->work_lock);   /* sync with update task */

                                                     ^^^^^^^^^^^^^^^^^^^^^

Keep this comment. It's useful.

drivers/net/ethernet/chelsio/cxgb/cxgb2.c static int cxgb_close(struct net_device *dev) · line 290

Quoted 5 lines

drivers/net/ethernet/chelsio/cxgb/cxgb2.c +1 -1

-		spin_unlock(&adapter->work_lock);
+		spin_unlock_wait(&adapter->work_lock);
 		cancel_mac_stats_update(adapter);
 	}

regards,
dan carpenter

Nicholas Mc Guire

author

2013-12-02 23:30 UTC #2 lore raw

In reply to Nicholas Mc Guire

Quoted 5 lines · Dan Carpenter

I think the patch is generally ok. It needs to be split up and sent through the individual maintainers. The changelog has a lot of extra verbiage.
On Mon, Dec 02, 2013 at 03:56:46PM +0100, Nicholas Mc Guire wrote:

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
From 5b3d0b88438846cbdd888798c2fb04e20c69e95d Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <[email protected]>
Date: Sun, 1 Dec 2013 22:53:18 -0500
Subject: [PATCH] consolidate spin_lock/unlock waiting with spin_unlock_wait
Don't include these bits in the patch and don't indent the changelog by one space (don't indent at all, it should be hard left).

will drop it - thought the output from git format-patch would be the right format - my missunderstanding.

Quoted 22 lines

Quoted 14 lines
Hi !
Not sure if this is the right place to put this... This patch proposes a small API consolidation.
in the following files there is a spin_lock/unlock in use for waiting on some possibly concurrent thread - that is there is a
	spin_lock(lock);
	spin_unlock(lock);
so in the contended case the calling thread will wait for any concurrent access.
Quoted 7 lines
arch/blackfin/mach-bf561/smp.c lock/unlock at lines 72 73 arch/arm/mach-sti/platsmp.c lock/unlock at lines 53 54 arch/cris/arch-v32/drivers/mach-fs/gpio.c lock/unlock at lines 764 765 drivers/staging/lustre/lustre/obdclass/llog_obd.c lock/unlock at lines 87 89 drivers/net/ethernet/chelsio/cxgb/cxgb2.c lock/unlock at lines 287 288 drivers/net/ethernet/ti/cpmac.c lock/unlock at lines 582 583 fs/fscache/object.c lock/unlock at lines 692 693

Quoted 2 lines

This block is not useful.

Quoted 6 lines

Quoted 6 lines
in
commit c2f21ce2e31286a0a32f8da0a7856e9ca1122ef3
Author: Thomas Gleixner <[email protected]>
Date: Wed Dec 2 20:02:59 2009 +0100

Quoted 6 lines

Quoted 2 lines
locking: Implement new raw_spinlock
This is not the patch which introduced spin_unlock_wait(). That function was around before we adopted git.

used git blame to find that - how would I find this information or is it not relevant to include it ?

Quoted 10 lines

Quoted 5 lines
the api was extended with include/linux/spinlock.h:spin_unlock_wait() for just this purpose (I think atleast) so the below patch changes these currently hard-coded spin_lock/unlock to use the available API. so as its in spinlock.h and the code in question is using spin_lock/unlock no new header file inclusion is needed.
We hopefully can assume you compile tested these so this information about header files is redundant. If you didn't compile test, then we will get really annoyed.

yes but only ran it on a 32 and 64 bit box to test them - but taht would at most cover the case in fs so that is very limited. What I did is generate the .i files and then looked at the function that got plugged in at that line e.g. arch_spin_unlock_wait in the fs/fscache/object.c then checked if that should be equivalent. in the arch/cris/arch-v32/drivers/mach-fs/gpio.c I contacted the maintainer of the file as I could not figure out what it was waiting on (and its UP only).

Quoted 7 lines

Quoted 4 lines
all the above listed cases are consecutive spin_lock/spin_unlock. the one-line offset in llog_obd.c at lines 87 89 is due to a comment not code so it also is a consecutive spin_lock/spin_unlock,
Obvious. Don't include this in the commit message.

Quoted 5 lines

Quoted 5 lines
If these changes make any sense should this go out to all of the code authors for review/ack or is this simple enough for it to just go out via kernel-janitors/LKML for review ?

Quoted 9 lines

Quoted 2 lines
patch is against 3.13.0-rc1 (linux-stable Greg KH tree)
Don't do patches against stable. Do them against the current -rc or against linux-next. In theory, the current -rc is better because then you can test them without other patches affecting the results, but in real life, you don't have the hardware for most of these and I doubt you are going to test the filesystem ones.

ok - will redo it again linux-next and send it out to the maintainers for review

Quoted 8 lines

drivers/net/ethernet/chelsio/cxgb/cxgb2.c static int cxgb_close(struct net_device *dev) · line 287

Quoted 5 lines

drivers/net/ethernet/chelsio/cxgb/cxgb2.c +0 -1

@@ -284,8 +284,7 @@ static int cxgb_close(struct net_device *dev)
 	    !(adapter->open_device_map & PORT_MASK)) {
 		/* Stop statistics accumulation. */
 		smp_mb__after_clear_bit();
-		spin_lock(&adapter->work_lock);   /* sync with update task */

                                                     ^^^^^^^^^^^^^^^^^^^^^

Keep this comment. It's useful.

thanks - comletely overlooked that I was removing information not just code

Quoted 6 lines

drivers/net/ethernet/chelsio/cxgb/cxgb2.c static int cxgb_close(struct net_device *dev) · line 290
Quoted 5 lines
drivers/net/ethernet/chelsio/cxgb/cxgb2.c +1 -1
-		spin_unlock(&adapter->work_lock);
+		spin_unlock_wait(&adapter->work_lock);
 		cancel_mac_stats_update(adapter);
 	}
 

thanks! will fixup split it and send it to the file maintainers.

thx!
hofrat

Dan Carpenter

2013-12-03 07:44 UTC #3 lore raw

In reply to Nicholas Mc Guire

Quoted 11 lines · Nicholas Mc Guire

Quoted 10 lines
Quoted 5 lines
the api was extended with include/linux/spinlock.h:spin_unlock_wait() for just this purpose (I think atleast) so the below patch changes these currently hard-coded spin_lock/unlock to use the available API. so as its in spinlock.h and the code in question is using spin_lock/unlock no new header file inclusion is needed.
We hopefully can assume you compile tested these so this information about header files is redundant. If you didn't compile test, then we will get really annoyed.

Quoted 8 lines

yes but only ran it on a 32 and 64 bit box to test them - but taht would at most cover the case in fs so that is very limited. What I did is generate the .i files and then looked at the function that got plugged in at that line e.g. arch_spin_unlock_wait in the fs/fscache/object.c then checked if that should be equivalent. in the arch/cris/arch-v32/drivers/mach-fs/gpio.c I contacted the maintainer of the file as I could not figure out what it was waiting on (and its UP only).

Really, in the case where a cleanup breaks the build people are going to be annoyed. The traditional thing to do is just to applogize ahead of time between the --- and the diffstat.

Please note: I don't think this breaks the build but I can't compile it myself.

regards,
dan carpenter

Dan Carpenter

2013-12-03 11:57 UTC #4 lore raw

In reply to Nicholas Mc Guire

Quoted 8 lines · Nicholas Mc Guire

Quoted 4 lines
This is not the patch which introduced spin_unlock_wait(). That function was around before we adopted git.
used git blame to find that - how would I find this information or is it not relevant to include it ?

If it's older than git then forget about it.

regards,
dan carpenter

Nicholas Mc Guire

author

2013-12-04 06:17 UTC #5 lore raw

In reply to Nicholas Mc Guire

Quoted 6 lines · Dan Carpenter

Quoted 5 lines · Nicholas Mc Guire
Quoted 4 lines
This is not the patch which introduced spin_unlock_wait(). That function was around before we adopted git.

Quoted 6 lines

Quoted 3 lines
used git blame to find that - how would I find this information or is it not relevant to include it ?
If it's older than git then forget about it.

ok - so I guess that was a bit naive then :)

cleaning up and will resubmit.

many thanks!
hofrat