untangle spinlock macros

PATCH In review Eric Sandeen 5 participants 13 messages Sep 11, 2007 – Sep 21, 2007 XFS filesystem

lore

Share & tools

lore

Eric Sandeen

author

2007-09-11 05:05 UTC #0 lore raw

PATCH 1/1 untangle spinlock macros

I have a series of patches at
http://sandeen.net/xfs-patches/patches-spinlock-unobfuscate.tar.bz2

to get rid of the macros upon macros hiding xfs' use of spinlocks, via for example AIL_LOCK->mutex_spinlock->spin_lock. This also gets rid of the unused "cookie" variables declared via SPLDECL(s) and other open-coded unsigned long s; declarations.

patches in the tarball, broken out by lock as requested a while ago by dgc:

unwrap_AIL_LOCK
unwrap_LOG_LOCK
unwrap_GRANT_LOCK
unwrap_XFS_DQ_PINUNLOCK
unwrap_pagb_lock
unwrap_xfs_dabuf_global_lock
unwrap_mru_lock
unwrap_XFS_SB_LOCK
no_kt_lock
cleanup_lock_goop

Patches have comments/descriptions/signed-off lines in them.

By the end of the series, spin.h is almost empty, only spin_lock_init / spinlock_destroy are left, and could maybe even be pulled out.... wasn't sure how far to go. Since the kernel has a mutex_destroy, I wonder if spinlocks will ever get similar treatment... anyway....

I can post them to the list individually if preferred... though it's fairly mechanical and not terribly interesting...

Thanks,
-Eric

Lachlan McIlroy

2007-09-12 01:04 UTC #1 lore raw

They look good to me. There's still a few unused variables left over but nothing we can't fix up.

Quoted 37 lines · Eric Sandeen

I have a series of patches at
http://sandeen.net/xfs-patches/patches-spinlock-unobfuscate.tar.bz2
to get rid of the macros upon macros hiding xfs' use of spinlocks, via for example AIL_LOCK->mutex_spinlock->spin_lock. This also gets rid of the unused "cookie" variables declared via SPLDECL(s) and other open-coded unsigned long s; declarations.
patches in the tarball, broken out by lock as requested a while ago by dgc:
unwrap_AIL_LOCK
unwrap_LOG_LOCK
unwrap_GRANT_LOCK
unwrap_XFS_DQ_PINUNLOCK
unwrap_pagb_lock
unwrap_xfs_dabuf_global_lock
unwrap_mru_lock
unwrap_XFS_SB_LOCK
no_kt_lock
cleanup_lock_goop
Patches have comments/descriptions/signed-off lines in them.
By the end of the series, spin.h is almost empty, only spin_lock_init / spinlock_destroy are left, and could maybe even be pulled out.... wasn't sure how far to go. Since the kernel has a mutex_destroy, I wonder if spinlocks will ever get similar treatment... anyway....
I can post them to the list individually if preferred... though it's fairly mechanical and not terribly interesting...
Thanks,
-Eric

Eric Sandeen

author

2007-09-12 01:35 UTC #2 lore raw

Quoted 2 lines · Lachlan McIlroy

They look good to me. There's still a few unused variables left over but nothing we can't fix up.

Great - and, weird - I don't see them in my tree... maybe I missed a quilt refresh, but I don't think so... odd.

-Eric

Lachlan McIlroy

2007-09-12 05:47 UTC #3 lore raw

Just these ones... not a problem.

fs/xfs/xfs_mount.c: In function ‘xfs_icsb_cpu_notify’:
fs/xfs/xfs_mount.c:1920: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_sync_counters_flags’:
fs/xfs/xfs_mount.c:2191: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_balance_counter’:
fs/xfs/xfs_mount.c:2249: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_modify_counters’:
fs/xfs/xfs_mount.c:2299: warning: unused variable ‘s’

Quoted 11 lines · Eric Sandeen

Quoted 2 lines · Lachlan McIlroy
They look good to me. There's still a few unused variables left over but nothing we can't fix up.
Great - and, weird - I don't see them in my tree... maybe I missed a quilt refresh, but I don't think so... odd.
-Eric

Eric Sandeen

author

2007-09-12 14:32 UTC #4 lore raw

Quoted 10 lines · Lachlan McIlroy

Just these ones... not a problem.
fs/xfs/xfs_mount.c: In function ‘xfs_icsb_cpu_notify’:
fs/xfs/xfs_mount.c:1920: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_sync_counters_flags’:
fs/xfs/xfs_mount.c:2191: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_balance_counter’:
fs/xfs/xfs_mount.c:2249: warning: unused variable ‘s’ fs/xfs/xfs_mount.c: In function ‘xfs_icsb_modify_counters’:
fs/xfs/xfs_mount.c:2299: warning: unused variable ‘s’

Ah, I didn't build with CONFIG_HOTPLUG_CPU, whoops!

Ok, I'll let you tidy that up ;-) and add it to my default config I guess!

-Eric

Donald Douwsma

2007-09-12 01:51 UTC #5 lore raw

In reply to Eric Sandeen

Quoted 8 lines · Eric Sandeen

I have a series of patches at
http://sandeen.net/xfs-patches/patches-spinlock-unobfuscate.tar.bz2
to get rid of the macros upon macros hiding xfs' use of spinlocks, via for example AIL_LOCK->mutex_spinlock->spin_lock. This also gets rid of the unused "cookie" variables declared via SPLDECL(s) and other open-coded unsigned long s; declarations.

Hi Eric,

Quoted 1 line

unwrap_AIL_LOCK

Here you change the comment to use the descriptive name

- * We must not be holding the AIL_LOCK at this point. Calling incore() to
- * search the buffer cache can be a time consuming thing, and AIL_LOCK is a
+ * We must not be holding the AIL lock at this point. Calling incore() to
+ * search the buffer cache can be a time consuming thing, and AIL lock is a
* spinlock.
*/

Quoted 7 lines

unwrap_LOG_LOCK
unwrap_GRANT_LOCK
unwrap_XFS_DQ_PINUNLOCK
unwrap_pagb_lock
unwrap_xfs_dabuf_global_lock
unwrap_mru_lock
unwrap_XFS_SB_LOCK

But here you use the name of the lock variable.

/*

- * We actually don't have to acquire the SB_LOCK at all.
+ * We actually don't have to acquire the m_sb_lock at all.
* This can only be called from mount, and that's single threaded. XXX */

Quoted 9 lines

no_kt_lock
cleanup_lock_goop
Patches have comments/descriptions/signed-off lines in them.
By the end of the series, spin.h is almost empty, only spin_lock_init / spinlock_destroy are left, and could maybe even be pulled out.... wasn't sure how far to go. Since the kernel has a mutex_destroy, I wonder if spinlocks will ever get similar treatment... anyway....

So the only things left in spin.h are the spinlock headers and

#define spinlock_init(lock, name) spin_lock_init(lock)
#define spinlock_destroy(lock)

I cant se why we need these abstractions. Should we nuke the whole file and add the spinlock headers elsewhere?

Don

Eric Sandeen

author

2007-09-12 01:55 UTC #6 lore raw

Quoted 22 lines · Donald Douwsma

Hi Eric,
Quoted 1 line
unwrap_AIL_LOCK
Here you change the comment to use the descriptive name
- * We must not be holding the AIL_LOCK at this point. Calling incore() to
- * search the buffer cache can be a time consuming thing, and AIL_LOCK is a
+ * We must not be holding the AIL lock at this point. Calling incore() to
+ * search the buffer cache can be a time consuming thing, and AIL lock is a
* spinlock.
*/
Quoted 7 lines
unwrap_LOG_LOCK
unwrap_GRANT_LOCK
unwrap_XFS_DQ_PINUNLOCK
unwrap_pagb_lock
unwrap_xfs_dabuf_global_lock
unwrap_mru_lock
unwrap_XFS_SB_LOCK
But here you use the name of the lock variable.
         /*

Review

Quoted 4 lines

(quoted diff) +1 -1

-        * We actually don't have to acquire the SB_LOCK at all.
+        * We actually don't have to acquire the m_sb_lock at all.
          * This can only be called from mount, and that's single threaded. XXX
          */

Hm yup. 2 different evenings, oops. ;-) Have a preference?

Quoted 9 lines

Quoted 9 lines
no_kt_lock
cleanup_lock_goop
Patches have comments/descriptions/signed-off lines in them.
By the end of the series, spin.h is almost empty, only spin_lock_init / spinlock_destroy are left, and could maybe even be pulled out.... wasn't sure how far to go. Since the kernel has a mutex_destroy, I wonder if spinlocks will ever get similar treatment... anyway....

Quoted 7 lines

So the only things left in spin.h are the spinlock headers and
  #define spinlock_init(lock, name)      spin_lock_init(lock)
  #define        spinlock_destroy(lock)
I cant se why we need these abstractions. Should we nuke the whole file and add the spinlock headers elsewhere?

We don't, really. It'd be easy to nuke the file and add them to xfs_linux.h.

I'll send a patch to do so if you like.

-Eric

Eric Sandeen

author

2007-09-12 02:07 UTC #7 lore raw

In reply to Donald Douwsma

Quoted 12 lines · Donald Douwsma

So the only things left in spin.h are the spinlock headers and
  #define spinlock_init(lock, name)      spin_lock_init(lock)
  #define        spinlock_destroy(lock)
I cant se why we need these abstractions. Should we nuke the whole file and add the spinlock headers elsewhere?
Don

Ok, if you want it :)

-------------------------

remove abstraction macros in spin.h, remove the callers, and remove the file.

Signed-off-by Eric Sandeen <[email protected]>

fs/xfs/linux-2.6/spin.h deleted +0 ~~-27~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/spin.h
===================================================================
@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-#ifndef __XFS_SUPPORT_SPIN_H__
-#define __XFS_SUPPORT_SPIN_H__
-
-#include <linux/sched.h>	/* preempt needs this */
-#include <linux/spinlock.h>
-
-#define spinlock_init(lock, name)	spin_lock_init(lock)
-#define	spinlock_destroy(lock)
-
-#endif /* __XFS_SUPPORT_SPIN_H__ */

fs/xfs/linux-2.6/xfs_buf.c +1 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
@@ -1568,7 +1568,7 @@ xfs_alloc_delwrite_queue(
 
 	INIT_LIST_HEAD(&btp->bt_list);
 	INIT_LIST_HEAD(&btp->bt_delwrite_queue);
-	spinlock_init(&btp->bt_delwrite_lock, "delwri_lock");
+	spin_lock_init(&btp->bt_delwrite_lock);
 	btp->bt_flags = 0;
 	btp->bt_task = kthread_run(xfsbufd, btp, "xfsbufd");
 	if (IS_ERR(btp->bt_task)) {

fs/xfs/linux-2.6/xfs_linux.h +1 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
@@ -43,7 +43,6 @@
 
 #include <kmem.h>
 #include <mrlock.h>
-#include <spin.h>
 #include <sv.h>
 #include <mutex.h>
 #include <sema.h>

@@ -75,6 +74,7 @@
 #include <linux/notifier.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>

fs/xfs/quota/xfs_qm.c +1 -2

Index: linux-2.6-xfs/fs/xfs/quota/xfs_qm.c
===================================================================
@@ -1131,7 +1131,7 @@ xfs_qm_init_quotainfo(
 		return error;
 	}
 
-	spinlock_init(&qinf->qi_pinlock, "xfs_qinf_pin");
+	spin_lock_init(&qinf->qi_pinlock);
 	xfs_qm_list_init(&qinf->qi_dqlist, "mpdqlist", 0);
 	qinf->qi_dqreclaims = 0;

@@ -1228,7 +1228,6 @@ xfs_qm_destroy_quotainfo(
 	 */
 	xfs_qm_rele_quotafs_ref(mp);
 
-	spinlock_destroy(&qi->qi_pinlock);
 	xfs_qm_list_destroy(&qi->qi_dqlist);
 
 	if (qi->qi_uquotaip) {

fs/xfs/support/debug.c +0 -1

Index: linux-2.6-xfs/fs/xfs/support/debug.c
===================================================================
@@ -17,7 +17,6 @@
  */
 #include <xfs.h>
 #include "debug.h"
-#include "spin.h"
 
 static char		message[1024];	/* keep it off the stack */
 static DEFINE_SPINLOCK(xfs_err_lock);

fs/xfs/support/ktrace.h +0 -2

Index: linux-2.6-xfs/fs/xfs/support/ktrace.h
===================================================================
@@ -18,8 +18,6 @@
 #ifndef __XFS_SUPPORT_KTRACE_H__
 #define __XFS_SUPPORT_KTRACE_H__
 
-#include <spin.h>
-
 /*
  * Trace buffer entry structure.
  */

fs/xfs/xfs_alloc.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_alloc.c
===================================================================
@@ -2206,7 +2206,7 @@ xfs_alloc_read_agf(
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNOi]);
 		pag->pagf_levels[XFS_BTNUM_CNTi] =
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]);
-		spinlock_init(&pag->pagb_lock, "xfspagb");
+		spin_lock_init(&pag->pagb_lock);
 		pag->pagb_list = kmem_zalloc(XFS_PAGB_NUM_SLOTS *
 					sizeof(xfs_perag_busy_t), KM_SLEEP);
 		pag->pagf_init = 1;

fs/xfs/xfs_log.c +2 -4

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================
@@ -1189,8 +1189,8 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
 	log->l_xbuf = bp;
 
-	spinlock_init(&log->l_icloglock, "iclog");
-	spinlock_init(&log->l_grant_lock, "grhead_iclog");
+	spin_lock_init(&log->l_icloglock);
+	spin_lock_init(&log->l_grant_lock);
 	initnsema(&log->l_flushsema, 0, "ic-flush");
 	xlog_state_ticket_alloc(log);  /* wait until after icloglock inited */

@@ -1543,8 +1543,6 @@ xlog_dealloc_log(xlog_t *log)
 		iclog = next_iclog;
 	}
 	freesema(&log->l_flushsema);
-	spinlock_destroy(&log->l_icloglock);
-	spinlock_destroy(&log->l_grant_lock);
 
 	/* XXXsup take a look at this again. */
 	if ((log->l_ticket_cnt != log->l_ticket_tcnt)  &&

fs/xfs/xfs_mount.c +3 -5

Index: linux-2.6-xfs/fs/xfs/xfs_mount.c
===================================================================
@@ -136,8 +136,8 @@ xfs_mount_init(void)
 		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
 	}
 
-	spinlock_init(&mp->m_ail_lock, "xfs_ail");
-	spinlock_init(&mp->m_sb_lock, "xfs_sb");
+	spin_lock_init(&mp->m_ail_lock);
+	spin_lock_init(&mp->m_sb_lock);
 	mutex_init(&mp->m_ilock);
 	mutex_init(&mp->m_growlock);
 	/*

@@ -171,8 +171,6 @@ xfs_mount_free(
 			  sizeof(xfs_perag_t) * mp->m_sb.sb_agcount);
 	}
 
-	spinlock_destroy(&mp->m_ail_lock);
-	spinlock_destroy(&mp->m_sb_lock);
 	mutex_destroy(&mp->m_ilock);
 	mutex_destroy(&mp->m_growlock);
 	if (mp->m_quotainfo)

@@ -616,7 +614,7 @@ xfs_mount_common(xfs_mount_t *mp, xfs_sb
 	int	i;
 
 	mp->m_agfrotor = mp->m_agirotor = 0;
-	spinlock_init(&mp->m_agirotor_lock, "m_agirotor_lock");
+	spin_lock_init(&mp->m_agirotor_lock);
 	mp->m_maxagi = mp->m_sb.sb_agcount;
 	mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG;
 	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;

fs/xfs/xfs_mru_cache.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_mru_cache.c
===================================================================
@@ -368,7 +368,7 @@ xfs_mru_cache_create(
 	 */
 	INIT_RADIX_TREE(&mru->store, GFP_ATOMIC);
 	INIT_LIST_HEAD(&mru->reap_list);
-	spinlock_init(&mru->lock, "xfs_mru_cache");
+	spin_lock_init(&mru->lock);
 	INIT_DELAYED_WORK(&mru->work, _xfs_mru_cache_reap);
 
 	mru->grp_time  = grp_time;

fs/xfs/xfs_vfsops.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
===================================================================
@@ -68,7 +68,7 @@ xfs_init(void)
 	extern kmem_zone_t	*xfs_dabuf_zone;
 #ifdef XFS_DABUF_DEBUG
 	extern spinlock_t        xfs_dabuf_global_lock;
-	spinlock_init(&xfs_dabuf_global_lock, "xfsda");
+	spin_lock_init(&xfs_dabuf_global_lock);
 #endif
 
 	/*

Lachlan McIlroy

2007-09-12 06:02 UTC #8 lore raw

These changes look good Eric.

I'm in two minds about losing the spinlock_destroy() macros though. If Linux ever implements a spinlock teardown routine it would be nice to still have all the placeholders still there. Although I can't imagine it would do any more than assert that the lock is not currently held. If someone else wants to lose the macros then I'm not going to argue.

Quoted 233 lines · Eric Sandeen

Quoted 12 lines · Donald Douwsma

So the only things left in spin.h are the spinlock headers and
  #define spinlock_init(lock, name)      spin_lock_init(lock)
  #define        spinlock_destroy(lock)
I cant se why we need these abstractions. Should we nuke the whole file and add the spinlock headers elsewhere?
Don

Ok, if you want it :)

-------------------------

remove abstraction macros in spin.h, remove the callers, and remove the file.

Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/spin.h
===================================================================

fs/xfs/linux-2.6/spin.h deleted +0 ~~-27~~

@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-#ifndef __XFS_SUPPORT_SPIN_H__
-#define __XFS_SUPPORT_SPIN_H__
-
-#include <linux/sched.h>	/* preempt needs this */
-#include <linux/spinlock.h>
-
-#define spinlock_init(lock, name)	spin_lock_init(lock)
-#define	spinlock_destroy(lock)
-
-#endif /* __XFS_SUPPORT_SPIN_H__ */

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c +1 -1

@@ -1568,7 +1568,7 @@ xfs_alloc_delwrite_queue(
 
 	INIT_LIST_HEAD(&btp->bt_list);
 	INIT_LIST_HEAD(&btp->bt_delwrite_queue);
-	spinlock_init(&btp->bt_delwrite_lock, "delwri_lock");
+	spin_lock_init(&btp->bt_delwrite_lock);
 	btp->bt_flags = 0;
 	btp->bt_task = kthread_run(xfsbufd, btp, "xfsbufd");
 	if (IS_ERR(btp->bt_task)) {

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_linux.h
===================================================================

fs/xfs/linux-2.6/xfs_linux.h +1 -1

@@ -43,7 +43,6 @@
 
 #include <kmem.h>
 #include <mrlock.h>
-#include <spin.h>
 #include <sv.h>
 #include <mutex.h>
 #include <sema.h>

@@ -75,6 +74,7 @@
 #include <linux/notifier.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>

Index: linux-2.6-xfs/fs/xfs/quota/xfs_qm.c
===================================================================

fs/xfs/quota/xfs_qm.c +1 -2

@@ -1131,7 +1131,7 @@ xfs_qm_init_quotainfo(
 		return error;
 	}
 
-	spinlock_init(&qinf->qi_pinlock, "xfs_qinf_pin");
+	spin_lock_init(&qinf->qi_pinlock);
 	xfs_qm_list_init(&qinf->qi_dqlist, "mpdqlist", 0);
 	qinf->qi_dqreclaims = 0;

@@ -1228,7 +1228,6 @@ xfs_qm_destroy_quotainfo(
 	 */
 	xfs_qm_rele_quotafs_ref(mp);
 
-	spinlock_destroy(&qi->qi_pinlock);
 	xfs_qm_list_destroy(&qi->qi_dqlist);
 
 	if (qi->qi_uquotaip) {

Index: linux-2.6-xfs/fs/xfs/support/debug.c
===================================================================

fs/xfs/support/debug.c +0 -1

@@ -17,7 +17,6 @@
  */
 #include <xfs.h>
 #include "debug.h"
-#include "spin.h"
 
 static char		message[1024];	/* keep it off the stack */
 static DEFINE_SPINLOCK(xfs_err_lock);

Index: linux-2.6-xfs/fs/xfs/support/ktrace.h
===================================================================

fs/xfs/support/ktrace.h +0 -2

@@ -18,8 +18,6 @@
 #ifndef __XFS_SUPPORT_KTRACE_H__
 #define __XFS_SUPPORT_KTRACE_H__
 
-#include <spin.h>
-
 /*
  * Trace buffer entry structure.
  */

Index: linux-2.6-xfs/fs/xfs/xfs_alloc.c
===================================================================

fs/xfs/xfs_alloc.c +1 -1

@@ -2206,7 +2206,7 @@ xfs_alloc_read_agf(
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNOi]);
 		pag->pagf_levels[XFS_BTNUM_CNTi] =
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]);
-		spinlock_init(&pag->pagb_lock, "xfspagb");
+		spin_lock_init(&pag->pagb_lock);
 		pag->pagb_list = kmem_zalloc(XFS_PAGB_NUM_SLOTS *
 					sizeof(xfs_perag_busy_t), KM_SLEEP);
 		pag->pagf_init = 1;

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================

fs/xfs/xfs_log.c +2 -4

@@ -1189,8 +1189,8 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
 	log->l_xbuf = bp;
 
-	spinlock_init(&log->l_icloglock, "iclog");
-	spinlock_init(&log->l_grant_lock, "grhead_iclog");
+	spin_lock_init(&log->l_icloglock);
+	spin_lock_init(&log->l_grant_lock);
 	initnsema(&log->l_flushsema, 0, "ic-flush");
 	xlog_state_ticket_alloc(log);  /* wait until after icloglock inited */

@@ -1543,8 +1543,6 @@ xlog_dealloc_log(xlog_t *log)
 		iclog = next_iclog;
 	}
 	freesema(&log->l_flushsema);
-	spinlock_destroy(&log->l_icloglock);
-	spinlock_destroy(&log->l_grant_lock);
 
 	/* XXXsup take a look at this again. */
 	if ((log->l_ticket_cnt != log->l_ticket_tcnt)  &&

Index: linux-2.6-xfs/fs/xfs/xfs_mount.c
===================================================================

fs/xfs/xfs_mount.c +3 -5

@@ -136,8 +136,8 @@ xfs_mount_init(void)
 		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
 	}
 
-	spinlock_init(&mp->m_ail_lock, "xfs_ail");
-	spinlock_init(&mp->m_sb_lock, "xfs_sb");
+	spin_lock_init(&mp->m_ail_lock);
+	spin_lock_init(&mp->m_sb_lock);
 	mutex_init(&mp->m_ilock);
 	mutex_init(&mp->m_growlock);
 	/*

@@ -171,8 +171,6 @@ xfs_mount_free(
 			  sizeof(xfs_perag_t) * mp->m_sb.sb_agcount);
 	}
 
-	spinlock_destroy(&mp->m_ail_lock);
-	spinlock_destroy(&mp->m_sb_lock);
 	mutex_destroy(&mp->m_ilock);
 	mutex_destroy(&mp->m_growlock);
 	if (mp->m_quotainfo)

@@ -616,7 +614,7 @@ xfs_mount_common(xfs_mount_t *mp, xfs_sb
 	int	i;
 
 	mp->m_agfrotor = mp->m_agirotor = 0;
-	spinlock_init(&mp->m_agirotor_lock, "m_agirotor_lock");
+	spin_lock_init(&mp->m_agirotor_lock);
 	mp->m_maxagi = mp->m_sb.sb_agcount;
 	mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG;
 	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;

Index: linux-2.6-xfs/fs/xfs/xfs_mru_cache.c
===================================================================

fs/xfs/xfs_mru_cache.c +1 -1

@@ -368,7 +368,7 @@ xfs_mru_cache_create(
 	 */
 	INIT_RADIX_TREE(&mru->store, GFP_ATOMIC);
 	INIT_LIST_HEAD(&mru->reap_list);
-	spinlock_init(&mru->lock, "xfs_mru_cache");
+	spin_lock_init(&mru->lock);
 	INIT_DELAYED_WORK(&mru->work, _xfs_mru_cache_reap);
 
 	mru->grp_time  = grp_time;

Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
===================================================================

fs/xfs/xfs_vfsops.c +1 -1

@@ -68,7 +68,7 @@ xfs_init(void)
 	extern kmem_zone_t	*xfs_dabuf_zone;
 #ifdef XFS_DABUF_DEBUG
 	extern spinlock_t        xfs_dabuf_global_lock;
-	spinlock_init(&xfs_dabuf_global_lock, "xfsda");
+	spin_lock_init(&xfs_dabuf_global_lock);
 #endif
 
 	/*

Christoph Hellwig

2007-09-12 08:57 UTC #9 lore raw

Quoted 10 lines · Lachlan McIlroy

These changes look good Eric.
I'm in two minds about losing the spinlock_destroy() macros though. If Linux
ever implements a spinlock teardown routine it would be nice to still have all
the placeholders still there. Although I can't imagine it would do any more than assert that the lock is not currently held. If someone else wants to lose
the macros then I'm not going to argue.

I'd say keep them for now. We don't need the spin.h header for them anyway, as single macro can simply move to xfs_linux.h

Eric Sandeen

author

2007-09-13 03:02 UTC #10 lore raw

Quoted 14 lines · Christoph Hellwig

Quoted 11 lines · Lachlan McIlroy
These changes look good Eric.
I'm in two minds about losing the spinlock_destroy() macros though. If Linux
ever implements a spinlock teardown routine it would be nice to still have all
the placeholders still there. Although I can't imagine it would do any more than assert that the lock is not currently held. If someone else wants to lose
the macros then I'm not going to argue.

Quoted 5 lines

I'd say keep them for now. We don't need the spin.h header for them anyway, as single macro can simply move to xfs_linux.h

Try this on for size.....

======================

remove spinlock init abstraction macro in spin.h, remove the callers, and remove the file. Move no-op spinlock_destroy to xfs_linux.h

Signed-off-by Eric Sandeen <[email protected]>

fs/xfs/linux-2.6/spin.h deleted +0 ~~-27~~

Index: linux-2.6-xfs/fs/xfs/linux-2.6/spin.h
===================================================================
@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-#ifndef __XFS_SUPPORT_SPIN_H__
-#define __XFS_SUPPORT_SPIN_H__
-
-#include <linux/sched.h>	/* preempt needs this */
-#include <linux/spinlock.h>
-
-#define spinlock_init(lock, name)	spin_lock_init(lock)
-#define	spinlock_destroy(lock)
-
-#endif /* __XFS_SUPPORT_SPIN_H__ */

fs/xfs/linux-2.6/xfs_buf.c +1 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================
@@ -1568,7 +1568,7 @@ xfs_alloc_delwrite_queue(
 
 	INIT_LIST_HEAD(&btp->bt_list);
 	INIT_LIST_HEAD(&btp->bt_delwrite_queue);
-	spinlock_init(&btp->bt_delwrite_lock, "delwri_lock");
+	spin_lock_init(&btp->bt_delwrite_lock);
 	btp->bt_flags = 0;
 	btp->bt_task = kthread_run(xfsbufd, btp, "xfsbufd");
 	if (IS_ERR(btp->bt_task)) {

fs/xfs/linux-2.6/xfs_linux.h +3 -1

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
@@ -43,7 +43,6 @@
 
 #include <kmem.h>
 #include <mrlock.h>
-#include <spin.h>
 #include <sv.h>
 #include <mutex.h>
 #include <sema.h>

@@ -75,6 +74,7 @@
 #include <linux/notifier.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>

@@ -145,6 +145,8 @@
 #define current_restore_flags_nested(sp, f)	\
 		(current->flags = ((current->flags & ~(f)) | (*(sp) & (f))))
 
+#define spinlock_destroy(lock)
+
 #define NBPP		PAGE_SIZE
 #define NDPP		(1 << (PAGE_SHIFT - 9))

fs/xfs/quota/xfs_qm.c +1 -1

Index: linux-2.6-xfs/fs/xfs/quota/xfs_qm.c
===================================================================
@@ -1131,7 +1131,7 @@ xfs_qm_init_quotainfo(
 		return error;
 	}
 
-	spinlock_init(&qinf->qi_pinlock, "xfs_qinf_pin");
+	spin_lock_init(&qinf->qi_pinlock);
 	xfs_qm_list_init(&qinf->qi_dqlist, "mpdqlist", 0);
 	qinf->qi_dqreclaims = 0;

fs/xfs/support/debug.c +0 -1

Index: linux-2.6-xfs/fs/xfs/support/debug.c
===================================================================
@@ -17,7 +17,6 @@
  */
 #include <xfs.h>
 #include "debug.h"
-#include "spin.h"
 
 static char		message[1024];	/* keep it off the stack */
 static DEFINE_SPINLOCK(xfs_err_lock);

fs/xfs/support/ktrace.h +0 -2

Index: linux-2.6-xfs/fs/xfs/support/ktrace.h
===================================================================
@@ -18,8 +18,6 @@
 #ifndef __XFS_SUPPORT_KTRACE_H__
 #define __XFS_SUPPORT_KTRACE_H__
 
-#include <spin.h>
-
 /*
  * Trace buffer entry structure.
  */

fs/xfs/xfs_alloc.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_alloc.c
===================================================================
@@ -2206,7 +2206,7 @@ xfs_alloc_read_agf(
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNOi]);
 		pag->pagf_levels[XFS_BTNUM_CNTi] =
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]);
-		spinlock_init(&pag->pagb_lock, "xfspagb");
+		spin_lock_init(&pag->pagb_lock);
 		pag->pagb_list = kmem_zalloc(XFS_PAGB_NUM_SLOTS *
 					sizeof(xfs_perag_busy_t), KM_SLEEP);
 		pag->pagf_init = 1;

fs/xfs/xfs_log.c +2 -2

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================
@@ -1189,8 +1189,8 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
 	log->l_xbuf = bp;
 
-	spinlock_init(&log->l_icloglock, "iclog");
-	spinlock_init(&log->l_grant_lock, "grhead_iclog");
+	spin_lock_init(&log->l_icloglock);
+	spin_lock_init(&log->l_grant_lock);
 	initnsema(&log->l_flushsema, 0, "ic-flush");
 	xlog_state_ticket_alloc(log);  /* wait until after icloglock inited */

fs/xfs/xfs_mount.c +3 -3

Index: linux-2.6-xfs/fs/xfs/xfs_mount.c
===================================================================
@@ -136,8 +136,8 @@ xfs_mount_init(void)
 		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
 	}
 
-	spinlock_init(&mp->m_ail_lock, "xfs_ail");
-	spinlock_init(&mp->m_sb_lock, "xfs_sb");
+	spin_lock_init(&mp->m_ail_lock);
+	spin_lock_init(&mp->m_sb_lock);
 	mutex_init(&mp->m_ilock);
 	mutex_init(&mp->m_growlock);
 	/*

@@ -616,7 +616,7 @@ xfs_mount_common(xfs_mount_t *mp, xfs_sb
 	int	i;
 
 	mp->m_agfrotor = mp->m_agirotor = 0;
-	spinlock_init(&mp->m_agirotor_lock, "m_agirotor_lock");
+	spin_lock_init(&mp->m_agirotor_lock);
 	mp->m_maxagi = mp->m_sb.sb_agcount;
 	mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG;
 	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;

fs/xfs/xfs_mru_cache.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_mru_cache.c
===================================================================
@@ -368,7 +368,7 @@ xfs_mru_cache_create(
 	 */
 	INIT_RADIX_TREE(&mru->store, GFP_ATOMIC);
 	INIT_LIST_HEAD(&mru->reap_list);
-	spinlock_init(&mru->lock, "xfs_mru_cache");
+	spin_lock_init(&mru->lock);
 	INIT_DELAYED_WORK(&mru->work, _xfs_mru_cache_reap);
 
 	mru->grp_time  = grp_time;

fs/xfs/xfs_vfsops.c +1 -1

Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
===================================================================
@@ -68,7 +68,7 @@ xfs_init(void)
 	extern kmem_zone_t	*xfs_dabuf_zone;
 #ifdef XFS_DABUF_DEBUG
 	extern spinlock_t        xfs_dabuf_global_lock;
-	spinlock_init(&xfs_dabuf_global_lock, "xfsda");
+	spin_lock_init(&xfs_dabuf_global_lock);
 #endif
 
 	/*

Russell Cattelan

2007-09-21 22:31 UTC #11 lore raw

Quoted 14 lines · Eric Sandeen

Quoted 12 lines · Christoph Hellwig
Quoted 9 lines · Lachlan McIlroy
These changes look good Eric.
I'm in two minds about losing the spinlock_destroy() macros though. If Linux ever implements a spinlock teardown routine it would be nice to still have all the placeholders still there. Although I can't imagine it would do any more than assert that the lock is not currently held. If someone else wants to lose the macros then I'm not going to argue.

Quoted 9 lines

Quoted 6 lines
I'd say keep them for now. We don't need the spin.h header for them anyway, as single macro can simply move to xfs_linux.h
Try this on for size.....

So this is a bit late now but please please leave the names they are useful for tracking locks. especially in the freebsd witness code.

Quoted 191 lines

======================

remove spinlock init abstraction macro in spin.h, remove the callers, and remove the file. Move no-op spinlock_destroy to xfs_linux.h

Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6-xfs/fs/xfs/linux-2.6/spin.h
===================================================================

fs/xfs/linux-2.6/spin.h deleted +0 ~~-27~~

@@ -1,27 +0,0 @@
-/*
- * Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it would be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write the Free Software Foundation,
- * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-#ifndef __XFS_SUPPORT_SPIN_H__
-#define __XFS_SUPPORT_SPIN_H__
-
-#include <linux/sched.h>	/* preempt needs this */
-#include <linux/spinlock.h>
-
-#define spinlock_init(lock, name)	spin_lock_init(lock)
-#define	spinlock_destroy(lock)
-
-#endif /* __XFS_SUPPORT_SPIN_H__ */

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_buf.c
===================================================================

fs/xfs/linux-2.6/xfs_buf.c +1 -1

@@ -1568,7 +1568,7 @@ xfs_alloc_delwrite_queue(
 
 	INIT_LIST_HEAD(&btp->bt_list);
 	INIT_LIST_HEAD(&btp->bt_delwrite_queue);
-	spinlock_init(&btp->bt_delwrite_lock, "delwri_lock");
+	spin_lock_init(&btp->bt_delwrite_lock);
 	btp->bt_flags = 0;
 	btp->bt_task = kthread_run(xfsbufd, btp, "xfsbufd");
 	if (IS_ERR(btp->bt_task)) {

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_linux.h
===================================================================

fs/xfs/linux-2.6/xfs_linux.h +3 -1

@@ -43,7 +43,6 @@
 
 #include <kmem.h>
 #include <mrlock.h>
-#include <spin.h>
 #include <sv.h>
 #include <mutex.h>
 #include <sema.h>

@@ -75,6 +74,7 @@
 #include <linux/notifier.h>
 #include <linux/delay.h>
 #include <linux/log2.h>
+#include <linux/spinlock.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>

@@ -145,6 +145,8 @@
 #define current_restore_flags_nested(sp, f)	\
 		(current->flags = ((current->flags & ~(f)) | (*(sp) & (f))))
 
+#define spinlock_destroy(lock)
+
 #define NBPP		PAGE_SIZE
 #define NDPP		(1 << (PAGE_SHIFT - 9))

Index: linux-2.6-xfs/fs/xfs/quota/xfs_qm.c
===================================================================

fs/xfs/quota/xfs_qm.c +1 -1

@@ -1131,7 +1131,7 @@ xfs_qm_init_quotainfo(
 		return error;
 	}
 
-	spinlock_init(&qinf->qi_pinlock, "xfs_qinf_pin");
+	spin_lock_init(&qinf->qi_pinlock);
 	xfs_qm_list_init(&qinf->qi_dqlist, "mpdqlist", 0);
 	qinf->qi_dqreclaims = 0;

Index: linux-2.6-xfs/fs/xfs/support/debug.c
===================================================================

fs/xfs/support/debug.c +0 -1

@@ -17,7 +17,6 @@
  */
 #include <xfs.h>
 #include "debug.h"
-#include "spin.h"
 
 static char		message[1024];	/* keep it off the stack */
 static DEFINE_SPINLOCK(xfs_err_lock);

Index: linux-2.6-xfs/fs/xfs/support/ktrace.h
===================================================================

fs/xfs/support/ktrace.h +0 -2

@@ -18,8 +18,6 @@
 #ifndef __XFS_SUPPORT_KTRACE_H__
 #define __XFS_SUPPORT_KTRACE_H__
 
-#include <spin.h>
-
 /*
  * Trace buffer entry structure.
  */

Index: linux-2.6-xfs/fs/xfs/xfs_alloc.c
===================================================================

fs/xfs/xfs_alloc.c +1 -1

@@ -2206,7 +2206,7 @@ xfs_alloc_read_agf(
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNOi]);
 		pag->pagf_levels[XFS_BTNUM_CNTi] =
 			be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]);
-		spinlock_init(&pag->pagb_lock, "xfspagb");
+		spin_lock_init(&pag->pagb_lock);
 		pag->pagb_list = kmem_zalloc(XFS_PAGB_NUM_SLOTS *
 					sizeof(xfs_perag_busy_t), KM_SLEEP);
 		pag->pagf_init = 1;

Index: linux-2.6-xfs/fs/xfs/xfs_log.c
===================================================================

fs/xfs/xfs_log.c +2 -2

@@ -1189,8 +1189,8 @@ xlog_alloc_log(xfs_mount_t	*mp,
 	ASSERT(XFS_BUF_VALUSEMA(bp) <= 0);
 	log->l_xbuf = bp;
 
-	spinlock_init(&log->l_icloglock, "iclog");
-	spinlock_init(&log->l_grant_lock, "grhead_iclog");
+	spin_lock_init(&log->l_icloglock);
+	spin_lock_init(&log->l_grant_lock);
 	initnsema(&log->l_flushsema, 0, "ic-flush");
 	xlog_state_ticket_alloc(log);  /* wait until after icloglock inited */

Index: linux-2.6-xfs/fs/xfs/xfs_mount.c
===================================================================

fs/xfs/xfs_mount.c +3 -3

@@ -136,8 +136,8 @@ xfs_mount_init(void)
 		mp->m_flags |= XFS_MOUNT_NO_PERCPU_SB;
 	}
 
-	spinlock_init(&mp->m_ail_lock, "xfs_ail");
-	spinlock_init(&mp->m_sb_lock, "xfs_sb");
+	spin_lock_init(&mp->m_ail_lock);
+	spin_lock_init(&mp->m_sb_lock);
 	mutex_init(&mp->m_ilock);
 	mutex_init(&mp->m_growlock);
 	/*

@@ -616,7 +616,7 @@ xfs_mount_common(xfs_mount_t *mp, xfs_sb
 	int	i;
 
 	mp->m_agfrotor = mp->m_agirotor = 0;
-	spinlock_init(&mp->m_agirotor_lock, "m_agirotor_lock");
+	spin_lock_init(&mp->m_agirotor_lock);
 	mp->m_maxagi = mp->m_sb.sb_agcount;
 	mp->m_blkbit_log = sbp->sb_blocklog + XFS_NBBYLOG;
 	mp->m_blkbb_log = sbp->sb_blocklog - BBSHIFT;

Index: linux-2.6-xfs/fs/xfs/xfs_mru_cache.c
===================================================================

fs/xfs/xfs_mru_cache.c +1 -1

@@ -368,7 +368,7 @@ xfs_mru_cache_create(
 	 */
 	INIT_RADIX_TREE(&mru->store, GFP_ATOMIC);
 	INIT_LIST_HEAD(&mru->reap_list);
-	spinlock_init(&mru->lock, "xfs_mru_cache");
+	spin_lock_init(&mru->lock);
 	INIT_DELAYED_WORK(&mru->work, _xfs_mru_cache_reap);
 
 	mru->grp_time  = grp_time;

Index: linux-2.6-xfs/fs/xfs/xfs_vfsops.c
===================================================================

fs/xfs/xfs_vfsops.c xfs_init(void) · line 74

Quoted 8 lines

fs/xfs/xfs_vfsops.c +1 -1

@@ -68,7 +68,7 @@ xfs_init(void)
 	extern kmem_zone_t	*xfs_dabuf_zone;
 #ifdef XFS_DABUF_DEBUG
 	extern spinlock_t        xfs_dabuf_global_lock;
-	spinlock_init(&xfs_dabuf_global_lock, "xfsda");
+	spin_lock_init(&xfs_dabuf_global_lock);
 #endif
 
 	/*

begin:vcard
fn:Russell Cattelan
n:Cattelan;Russell
email;internet:[email protected]
x-mozilla-html:FALSE
version:2.1
end:vcard

Eric Sandeen

author

2007-09-13 03:04 UTC #12 lore raw

In reply to Donald Douwsma

Review

Quoted 7 lines · Donald Douwsma

But here you use the name of the lock variable.

/*

(quoted diff) +1 -1

-        * We actually don't have to acquire the SB_LOCK at all.
+        * We actually don't have to acquire the m_sb_lock at all.
          * This can only be called from mount, and that's single threaded. XXX
          */

I was going to change this, but "sb lock" sounds way too generic to me...

-Eric