Skip to contentSkip to conversation

byteorder: introduce le32_add_cpu & friends to core

RFC PATCH In review Marcin Slusarz 3 participants 7 messages Dec 30, 2007 – Dec 31, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 byteorder: introduce le32_add_cpu & friends to core

There are many places where these functions would be useful.
(just look at: grep -r 'cpu_to_[ble12346]*([ble12346]*_to_cpu.*[-+]' linux-src/) What do you think?

ps: this patch depends on http://lkml.org/lkml/2007/12/25/35
--

add inline functions which add native byte order variable to little/big endian variable to core header and as an example convert ext3 to use them

le16_add_cpu(__le16 *var, u16 val)
le32_add_cpu(__le32 *var, u32 val)
le64_add_cpu(__le64 *var, u64 val)
be16_add_cpu(__be16 *var, u16 val)
be32_add_cpu(__be32 *var, u32 val)
be64_add_cpu(__be64 *var, u64 val)

---
 fs/ext3/balloc.c                  |    7 +----
 fs/ext3/ialloc.c                  |   12 +++------
 fs/ext3/resize.c                  |   12 +++------
 fs/ext3/super.c                   |    2 +-
 fs/ext3/xattr.c                   |    6 +---
 fs/ocfs2/cluster/endian.h         |   30 ------------------------
 fs/ocfs2/cluster/nodemanager.c    |    1 -
 fs/ocfs2/dlm/dlmast.c             |    1 -
 fs/ocfs2/endian.h                 |   45 -------------------------------------
 fs/ocfs2/ocfs2.h                  |    1 -
 include/linux/byteorder/generic.h |   30 ++++++++++++++++++++++++
 11 files changed, 43 insertions(+), 104 deletions(-)
 delete mode 100644 fs/ocfs2/cluster/endian.h
 delete mode 100644 fs/ocfs2/endian.h
fs/ext3/balloc.c +2 -5
index a8ba7e8..89f320f 100644
@@ -566,9 +566,7 @@ do_more:
 	jbd_unlock_bh_state(bitmap_bh);
 
 	spin_lock(sb_bgl_lock(sbi, block_group));
-	desc->bg_free_blocks_count =
-		cpu_to_le16(le16_to_cpu(desc->bg_free_blocks_count) +
-			group_freed);
+	le16_add_cpu(&desc->bg_free_blocks_count, group_freed);
 	spin_unlock(sb_bgl_lock(sbi, block_group));
 	percpu_counter_add(&sbi->s_freeblocks_counter, count);
 
@@ -1630,8 +1628,7 @@ allocated:
 			ret_block, goal_hits, goal_attempts);
 
 	spin_lock(sb_bgl_lock(sbi, group_no));
-	gdp->bg_free_blocks_count =
-			cpu_to_le16(le16_to_cpu(gdp->bg_free_blocks_count)-num);
+	le16_add_cpu(&gdp->bg_free_blocks_count, -num);
 	spin_unlock(sb_bgl_lock(sbi, group_no));
 	percpu_counter_sub(&sbi->s_freeblocks_counter, num);
 
fs/ext3/ialloc.c +4 -8
index 1bc8cd8..3b6b9da 100644
@@ -164,11 +164,9 @@ void ext3_free_inode (handle_t *handle, struct inode * inode)
 
 		if (gdp) {
 			spin_lock(sb_bgl_lock(sbi, block_group));
-			gdp->bg_free_inodes_count = cpu_to_le16(
-				le16_to_cpu(gdp->bg_free_inodes_count) + 1);
+			le16_add_cpu(&gdp->bg_free_inodes_count, 1);
 			if (is_directory)
-				gdp->bg_used_dirs_count = cpu_to_le16(
-				  le16_to_cpu(gdp->bg_used_dirs_count) - 1);
+				le16_add_cpu(&gdp->bg_used_dirs_count, -1);
 			spin_unlock(sb_bgl_lock(sbi, block_group));
 			percpu_counter_inc(&sbi->s_freeinodes_counter);
 			if (is_directory)
@@ -527,11 +525,9 @@ got:
 	err = ext3_journal_get_write_access(handle, bh2);
 	if (err) goto fail;
 	spin_lock(sb_bgl_lock(sbi, group));
-	gdp->bg_free_inodes_count =
-		cpu_to_le16(le16_to_cpu(gdp->bg_free_inodes_count) - 1);
+	le16_add_cpu(&gdp->bg_free_inodes_count, -1);
 	if (S_ISDIR(mode)) {
-		gdp->bg_used_dirs_count =
-			cpu_to_le16(le16_to_cpu(gdp->bg_used_dirs_count) + 1);
+		le16_add_cpu(&gdp->bg_used_dirs_count, 1);
 	}
 	spin_unlock(sb_bgl_lock(sbi, group));
 	BUFFER_TRACE(bh2, "call ext3_journal_dirty_metadata");
fs/ext3/resize.c +4 -8
index 44de145..5b402a7 100644
@@ -518,8 +518,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
 	EXT3_SB(sb)->s_gdb_count++;
 	kfree(o_group_desc);
 
-	es->s_reserved_gdt_blocks =
-		cpu_to_le16(le16_to_cpu(es->s_reserved_gdt_blocks) - 1);
+	le16_add_cpu(&es->s_reserved_gdt_blocks, -1);
 	ext3_journal_dirty_metadata(handle, EXT3_SB(sb)->s_sbh);
 
 	return 0;
@@ -891,10 +890,8 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input)
 	 * blocks/inodes before the group is live won't actually let us
 	 * allocate the new space yet.
 	 */
-	es->s_blocks_count = cpu_to_le32(le32_to_cpu(es->s_blocks_count) +
-		input->blocks_count);
-	es->s_inodes_count = cpu_to_le32(le32_to_cpu(es->s_inodes_count) +
-		EXT3_INODES_PER_GROUP(sb));
+	le32_add_cpu(&es->s_blocks_count, input->blocks_count);
+	le32_add_cpu(&es->s_inodes_count, EXT3_INODES_PER_GROUP(sb));
 
 	/*
 	 * We need to protect s_groups_count against other CPUs seeing
@@ -927,8 +924,7 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input)
 
 	/* Update the reserved block counts only once the new group is
 	 * active. */
-	es->s_r_blocks_count = cpu_to_le32(le32_to_cpu(es->s_r_blocks_count) +
-		input->reserved_blocks);
+	le32_add_cpu(&es->s_r_blocks_count, input->reserved_blocks);
 
 	/* Update the free space counts */
 	percpu_counter_add(&sbi->s_freeblocks_counter,
fs/ext3/super.c +1 -1
index 92f03b6..70da5a3 100644
@@ -1224,7 +1224,7 @@ static int ext3_setup_super(struct super_block *sb, struct ext3_super_block *es,
 #endif
 	if (!(__s16) le16_to_cpu(es->s_max_mnt_count))
 		es->s_max_mnt_count = cpu_to_le16(EXT3_DFL_MAX_MNT_COUNT);
-	es->s_mnt_count=cpu_to_le16(le16_to_cpu(es->s_mnt_count) + 1);
+	le16_add_cpu(&es->s_mnt_count, 1);
 	es->s_mtime = cpu_to_le32(get_seconds());
 	ext3_update_dynamic_rev(sb);
 	EXT3_SET_INCOMPAT_FEATURE(sb, EXT3_FEATURE_INCOMPAT_RECOVER);
fs/ext3/xattr.c +2 -4
index 4083738..fb89c29 100644
@@ -492,8 +492,7 @@ ext3_xattr_release_block(handle_t *handle, struct inode *inode,
 		get_bh(bh);
 		ext3_forget(handle, 1, inode, bh, bh->b_blocknr);
 	} else {
-		BHDR(bh)->h_refcount = cpu_to_le32(
-				le32_to_cpu(BHDR(bh)->h_refcount) - 1);
+		le32_add_cpu(&BHDR(bh)->h_refcount, -1);
 		error = ext3_journal_dirty_metadata(handle, bh);
 		if (IS_SYNC(inode))
 			handle->h_sync = 1;
@@ -780,8 +779,7 @@ inserted:
 				if (error)
 					goto cleanup_dquot;
 				lock_buffer(new_bh);
-				BHDR(new_bh)->h_refcount = cpu_to_le32(1 +
-					le32_to_cpu(BHDR(new_bh)->h_refcount));
+				le32_add_cpu(&BHDR(new_bh)->h_refcount, 1);
 				ea_bdebug(new_bh, "reusing; refcount now=%d",
 					le32_to_cpu(BHDR(new_bh)->h_refcount));
 				unlock_buffer(new_bh);
fs/ocfs2/cluster/endian.h deleted +0 -30
deleted file mode 100644
index 2df9082..0000000
@@ -1,30 +0,0 @@
-/* -*- mode: c; c-basic-offset: 8; -*-
- * vim: noexpandtab sw=8 ts=8 sts=0:
- *
- * Copyright (C) 2005 Oracle.  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; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will 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 to the
- * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
- * Boston, MA 021110-1307, USA.
- */
-
-#ifndef OCFS2_CLUSTER_ENDIAN_H
-#define OCFS2_CLUSTER_ENDIAN_H
-
-static inline void be32_add_cpu(__be32 *var, u32 val)
-{
-	*var = cpu_to_be32(be32_to_cpu(*var) + val);
-}
-
-#endif /* OCFS2_CLUSTER_ENDIAN_H */
fs/ocfs2/cluster/nodemanager.c +0 -1
index af2070d..709fba2 100644
@@ -24,7 +24,6 @@
 #include <linux/sysctl.h>
 #include <linux/configfs.h>
 
-#include "endian.h"
 #include "tcp.h"
 #include "nodemanager.h"
 #include "heartbeat.h"
fs/ocfs2/dlm/dlmast.c +0 -1
index 2fd8bde..644bee5 100644
@@ -43,7 +43,6 @@
 #include "cluster/heartbeat.h"
 #include "cluster/nodemanager.h"
 #include "cluster/tcp.h"
-#include "cluster/endian.h"
 
 #include "dlmapi.h"
 #include "dlmcommon.h"
fs/ocfs2/endian.h deleted +0 -45

45 lines not on this page · raw message

fs/ocfs2/ocfs2.h +0 -1
index 60a23e1..cda15fd 100644
@@ -43,7 +43,6 @@
 #include "dlm/dlmapi.h"
 
 #include "ocfs2_fs.h"
-#include "endian.h"
 #include "ocfs2_lockid.h"
 
 /* Most user visible OCFS2 inodes will have very few pieces of
include/linux/byteorder/generic.h +30 -0
index 3dc715b..d377155 100644
@@ -146,6 +146,36 @@
 #define htons(x) ___htons(x)
 #define ntohs(x) ___ntohs(x)
 
+static inline void le16_add_cpu(__le16 *var, u16 val)
+{
+	*var = cpu_to_le16(le16_to_cpu(*var) + val);
+}
+
+static inline void le32_add_cpu(__le32 *var, u32 val)
+{
+	*var = cpu_to_le32(le32_to_cpu(*var) + val);
+}
+
+static inline void le64_add_cpu(__le64 *var, u64 val)
+{
+	*var = cpu_to_le64(le64_to_cpu(*var) + val);
+}
+
+static inline void be16_add_cpu(__be16 *var, u16 val)
+{
+	*var = cpu_to_be16(be16_to_cpu(*var) + val);
+}
+
+static inline void be32_add_cpu(__be32 *var, u32 val)
+{
+	*var = cpu_to_be32(be32_to_cpu(*var) + val);
+}
+
+static inline void be64_add_cpu(__be64 *var, u64 val)
+{
+	*var = cpu_to_be64(be64_to_cpu(*var) + val);
+}
+
 #endif /* KERNEL */
 
 #endif /* _LINUX_BYTEORDER_GENERIC_H */
Signature · 1 line
1.5.3.7
#1 lore raw
Quoted 10 lines · Marcin Slusarz

There are many places where these functions would be useful.
(just look at: grep -r 'cpu_to_[ble12346]*([ble12346]*_to_cpu.*[-+]' linux-src/) What do you think?

ps: this patch depends on http://lkml.org/lkml/2007/12/25/35
--

add inline functions which add native byte order variable to little/big endian variable to core header and as an example convert ext3 to use them

Various places already have this as be*_add / le*_add, so it might be more useful to keep those names already in use.

#2 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · Marcin Slusarz

There are many places where these functions would be useful.
(just look at: grep -r 'cpu_to_[ble12346]*([ble12346]*_to_cpu.*[-+]' linux-src/) What do you think?

ps: this patch depends on http://lkml.org/lkml/2007/12/25/35
--

Quoted 6 lines
Quoted 3 lines

add inline functions which add native byte order variable to little/big endian variable to core header and as an example convert ext3 to use them

Various places already have this as be*_add / le*_add, so it might be more useful to keep those names already in use.

I found it in XFS only. Did I miss something?
be32_add is shorter than be32_add_cpu but I think it's not clear whether second parameter is in native byte order or not.

Marcin

#3 lore raw
Quoted 1 line · Marcin Slusarz

I found it in XFS only. Did I miss something?

These helpers come from ocfs2 which has both be and le variants.

Quoted 2 lines

be32_add is shorter than be32_add_cpu but I think it's not clear whether second parameter is in native byte order or not.

Then again adding foreing endian values doesn't make much sense.

If you insist on your naming please make sure to convert ocfs and xfs to your naming.

#5 lore raw
In reply to Marcin Slusarz
Quoted 10 lines · Marcin Slusarz

There are many places where these functions would be useful.
(just look at: grep -r 'cpu_to_[ble12346]*([ble12346]*_to_cpu.*[-+]' linux-src/) What do you think?

ps: this patch depends on http://lkml.org/lkml/2007/12/25/35
--

add inline functions which add native byte order variable to little/big endian variable to core header and as an example convert ext3 to use them

You might want to note where you got these functions from in your message.

Also, it would be easier to follow these changes if you had broken things up into two patches - one which moved stuff from ocfs2 into generic helpers and a 2nd one to convert ext3.

Looking into my crystal ball, I see a bunch of "convert to using byteorder math macros" patches coming down the pipe. Since we're talking about disk fields where a mistake could be costly, I suggest that any patch more than a couple of lines should be tested by the submitter with sparse. A statement that sparse didn't produce any new warnings (with the patch applied) could be included in the description.

	--Mark

--
Mark Fasheh
Principal Software Developer, Oracle
[email protected]

#4 lore raw
In reply to Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines

be32_add is shorter than be32_add_cpu but I think it's not clear whether second parameter is in native byte order or not.

Then again adding foreing endian values doesn't make much sense.

If you insist on your naming please make sure to convert ocfs and xfs to your naming.

Personally, I'd rather stick with the (IMHO) more descriptive name - btw, the ones he picked don't require any extra conversion in Ocfs2.

	--Mark

--
Mark Fasheh
Principal Software Developer, Oracle
[email protected]

#6 lore raw
In reply to Mark Fasheh
Quoted 5 lines · Mark Fasheh
Quoted 4 lines · Marcin Slusarz

There are many places where these functions would be useful.
(just look at: grep -r 'cpu_to_[ble12346]*([ble12346]*_to_cpu.*[-+]' linux-src/) What do you think?

Quoted 8 lines
Quoted 6 lines

ps: this patch depends on http://lkml.org/lkml/2007/12/25/35
--

add inline functions which add native byte order variable to little/big endian variable to core header and as an example convert ext3 to use them

You might want to note where you got these functions from in your message.

Yes, my mistake, sorry about that.

Quoted 3 lines

Also, it would be easier to follow these changes if you had broken things up into two patches - one which moved stuff from ocfs2 into generic helpers and a 2nd one to convert ext3.

Will do (tomorrow).

Quoted 6 lines

Looking into my crystal ball, I see a bunch of "convert to using byteorder math macros" patches coming down the pipe. Since we're talking about disk fields where a mistake could be costly, I suggest that any patch more than a couple of lines should be tested by the submitter with sparse. A statement that sparse didn't produce any new warnings (with the patch applied) could be included in the description.

Good idea. Thanks.

Marcin