Modified XArray entry bit flags as macro constants

Commit Message

JaeJoon Jung May 24, 2024, 2:49 a.m. UTC

From: Jung-JaeJoon <[e-mail]>

It would be better to modify the operation on the last two bits of the entry 
with a macro constant name rather than using a numeric constant.

#define XA_VALUE_ENTRY		1UL
#define XA_INTERNAL_ENTRY	2UL
#define XA_POINTER_ENTRY	3UL

In particular, in the xa_to_node() function, it is more consistent and efficient 
to perform a logical AND operation as shown below than a subtraction operation.

- return (struct xa_node *)((unsigned long)entry - 2);
+ return (struct xa_node *)((unsigned long)entry & ~XA_INTERNAL_ENTRY);

Additionally, it is better to modify the if condition below 
in the mas_store_root() function of lib/maple_tree.c to the xa_is_internal() inline function.

- else if (((unsigned long) (entry) & 3) == 2)
+ else if (xa_is_internal(entry))

And there is no reason to declare XA_CHECK_SCHED as an enum data type.
-enum {
-	XA_CHECK_SCHED = 4096,
-};
+#define XA_CHECK_SCHED          4096

Signed-off-by: JaeJoon Jung <[e-mail]>

---
 include/linux/xarray.h | 55 ++++++++++++++++++++++--------------------
 lib/maple_tree.c       |  2 +-
 2 files changed, 30 insertions(+), 27 deletions(-)

Comments

Matthew Wilcox May 24, 2024, 3:05 a.m. UTC | #1

On Fri, May 24, 2024 at 11:49:45AM +0900, Jung-JaeJoon wrote:
> From: Jung-JaeJoon <[e-mail]>
> 
> It would be better to modify the operation on the last two bits of the entry 
> with a macro constant name rather than using a numeric constant.
> 
> #define XA_VALUE_ENTRY		1UL
> #define XA_INTERNAL_ENTRY	2UL
> #define XA_POINTER_ENTRY	3UL
> 
> In particular, in the xa_to_node() function, it is more consistent and efficient 
> to perform a logical AND operation as shown below than a subtraction operation.
> 
> - return (struct xa_node *)((unsigned long)entry - 2);
> + return (struct xa_node *)((unsigned long)entry & ~XA_INTERNAL_ENTRY);
> 
> Additionally, it is better to modify the if condition below 
> in the mas_store_root() function of lib/maple_tree.c to the xa_is_internal() inline function.
> 
> - else if (((unsigned long) (entry) & 3) == 2)
> + else if (xa_is_internal(entry))
> 
> And there is no reason to declare XA_CHECK_SCHED as an enum data type.
> -enum {
> -	XA_CHECK_SCHED = 4096,
> -};
> +#define XA_CHECK_SCHED          4096

Thank you for your patch.  I agree with none of this.  Rejected.

Patch

diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index cb571dfcf4b1..d73dfe35a005 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -42,6 +42,16 @@ 
  * returned by the normal API.
  */
 
+#define XA_VALUE_ENTRY          1UL
+#define XA_INTERNAL_ENTRY       2UL
+#define XA_POINTER_ENTRY        3UL
+
+/*
+ * If iterating while holding a lock, drop the lock and reschedule
+ * every %XA_CHECK_SCHED loops.
+ */
+#define XA_CHECK_SCHED          4096
+
 #define BITS_PER_XA_VALUE	(BITS_PER_LONG - 1)
 
 /**
@@ -54,7 +64,7 @@ 
 static inline void *xa_mk_value(unsigned long v)
 {
 	WARN_ON((long)v < 0);
-	return (void *)((v << 1) | 1);
+	return (void *)((v << XA_VALUE_ENTRY) | XA_VALUE_ENTRY);
 }
 
 /**
@@ -66,7 +76,7 @@  static inline void *xa_mk_value(unsigned long v)
  */
 static inline unsigned long xa_to_value(const void *entry)
 {
-	return (unsigned long)entry >> 1;
+	return (unsigned long)entry >> XA_VALUE_ENTRY;
 }
 
 /**
@@ -78,7 +88,7 @@  static inline unsigned long xa_to_value(const void *entry)
  */
 static inline bool xa_is_value(const void *entry)
 {
-	return (unsigned long)entry & 1;
+	return (unsigned long)entry & XA_VALUE_ENTRY;
 }
 
 /**
@@ -111,7 +121,7 @@  static inline void *xa_tag_pointer(void *p, unsigned long tag)
  */
 static inline void *xa_untag_pointer(void *entry)
 {
-	return (void *)((unsigned long)entry & ~3UL);
+	return (void *)((unsigned long)entry & ~XA_POINTER_ENTRY);
 }
 
 /**
@@ -126,7 +136,7 @@  static inline void *xa_untag_pointer(void *entry)
  */
 static inline unsigned int xa_pointer_tag(void *entry)
 {
-	return (unsigned long)entry & 3UL;
+	return (unsigned long)entry & XA_POINTER_ENTRY;
 }
 
 /*
@@ -144,7 +154,7 @@  static inline unsigned int xa_pointer_tag(void *entry)
  */
 static inline void *xa_mk_internal(unsigned long v)
 {
-	return (void *)((v << 2) | 2);
+	return (void *)((v << XA_INTERNAL_ENTRY) | XA_INTERNAL_ENTRY);
 }
 
 /*
@@ -156,7 +166,7 @@  static inline void *xa_mk_internal(unsigned long v)
  */
 static inline unsigned long xa_to_internal(const void *entry)
 {
-	return (unsigned long)entry >> 2;
+	return (unsigned long)entry >> XA_INTERNAL_ENTRY;
 }
 
 /*
@@ -168,7 +178,7 @@  static inline unsigned long xa_to_internal(const void *entry)
  */
 static inline bool xa_is_internal(const void *entry)
 {
-	return ((unsigned long)entry & 3) == 2;
+	return ((unsigned long)entry & XA_POINTER_ENTRY) == XA_INTERNAL_ENTRY;
 }
 
 #define XA_ZERO_ENTRY		xa_mk_internal(257)
@@ -220,7 +230,7 @@  static inline int xa_err(void *entry)
 {
 	/* xa_to_internal() would not do sign extension. */
 	if (xa_is_err(entry))
-		return (long)entry >> 2;
+		return (long)entry >> XA_INTERNAL_ENTRY;
 	return 0;
 }
 
@@ -1245,19 +1255,19 @@  static inline struct xa_node *xa_parent_locked(const struct xarray *xa,
 /* Private */
 static inline void *xa_mk_node(const struct xa_node *node)
 {
-	return (void *)((unsigned long)node | 2);
+	return (void *)((unsigned long)node | XA_INTERNAL_ENTRY);
 }
 
 /* Private */
 static inline struct xa_node *xa_to_node(const void *entry)
 {
-	return (struct xa_node *)((unsigned long)entry - 2);
+	return (struct xa_node *)((unsigned long)entry & ~XA_INTERNAL_ENTRY);
 }
 
 /* Private */
 static inline bool xa_is_node(const void *entry)
 {
-	return xa_is_internal(entry) && (unsigned long)entry > 4096;
+	return xa_is_internal(entry) && (unsigned long)entry > XA_CHECK_SCHED;
 }
 
 /* Private */
@@ -1358,9 +1368,10 @@  struct xa_state {
  * We encode errnos in the xas->xa_node.  If an error has happened, we need to
  * drop the lock to fix it, and once we've done so the xa_state is invalid.
  */
-#define XA_ERROR(errno) ((struct xa_node *)(((unsigned long)errno << 2) | 2UL))
-#define XAS_BOUNDS	((struct xa_node *)1UL)
-#define XAS_RESTART	((struct xa_node *)3UL)
+#define XA_ERROR(errno) ((struct xa_node *)             \
+        (((unsigned long)errno << XA_INTERNAL_ENTRY) | XA_INTERNAL_ENTRY))
+#define XAS_BOUNDS	((struct xa_node *)XA_VALUE_ENTRY)
+#define XAS_RESTART	((struct xa_node *)XA_POINTER_ENTRY)
 
 #define __XA_STATE(array, index, shift, sibs)  {	\
 	.xa = array,					\
@@ -1449,7 +1460,7 @@  static inline void xas_set_err(struct xa_state *xas, long err)
  */
 static inline bool xas_invalid(const struct xa_state *xas)
 {
-	return (unsigned long)xas->xa_node & 3;
+	return (unsigned long)xas->xa_node & XA_POINTER_ENTRY;
 }
 
 /**
@@ -1477,13 +1488,13 @@  static inline bool xas_is_node(const struct xa_state *xas)
 /* True if the pointer is something other than a node */
 static inline bool xas_not_node(struct xa_node *node)
 {
-	return ((unsigned long)node & 3) || !node;
+	return ((unsigned long)node & XA_POINTER_ENTRY) || !node;
 }
 
 /* True if the node represents RESTART or an error */
 static inline bool xas_frozen(struct xa_node *node)
 {
-	return (unsigned long)node & 2;
+	return (unsigned long)node & XA_INTERNAL_ENTRY;
 }
 
 /* True if the node represents head-of-tree, RESTART or BOUNDS */
@@ -1764,14 +1775,6 @@  static inline void *xas_next_marked(struct xa_state *xas, unsigned long max,
 	return entry;
 }
 
-/*
- * If iterating while holding a lock, drop the lock and reschedule
- * every %XA_CHECK_SCHED loops.
- */
-enum {
-	XA_CHECK_SCHED = 4096,
-};
-
 /**
  * xas_for_each() - Iterate over a range of an XArray.
  * @xas: XArray operation state.
diff --git a/lib/maple_tree.c b/lib/maple_tree.c
index 2d7d27e6ae3c..c08545f8b09b 100644
--- a/lib/maple_tree.c
+++ b/lib/maple_tree.c
@@ -3515,7 +3515,7 @@  static inline void mas_store_root(struct ma_state *mas, void *entry)
 {
 	if (likely((mas->last != 0) || (mas->index != 0)))
 		mas_root_expand(mas, entry);
-	else if (((unsigned long) (entry) & 3) == 2)
+        else if (xa_is_internal(entry))
 		mas_root_expand(mas, entry);
 	else {
 		rcu_assign_pointer(mas->tree->ma_root, entry);