Skip to contentSkip to conversation

A few cleanups and refactorings, sync w/ upstream

PATCH Applied Anton Vorontsov 2 participants 7 messages Oct 4, 2012 – Oct 12, 2012 Memory management

lore
Share & tools
lore

Anton Vorontsov

author
#0 lore raw

cover A few cleanups and refactorings, sync w/ upstream

Hello Pekka,

Just a few updates to vmevents:

- Some cleanups and refactorings -- needed for easier integration of 'memory pressure' work;
- Forward to newer Linus' tree, fix conflicts.

For convenience, the merge commit and all the patches can be pulled from this repo:

git://git.infradead.org/users/cbou/linux-vmevent.git tags/vmevent-updates

Thanks,
Anton.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#1 lore raw

PATCH 1/3 vmevent: Remove unused code

struct vmevent_watch_event and a few definitions are not used anywhere, so let's remove them.

Signed-off-by Anton Vorontsov <[email protected]>
---
 mm/vmevent.c | 10 ----------
 1 file changed, 10 deletions(-)
mm/vmevent.c +0 -10
index a7f1042..39ef786 100644
@@ -11,16 +11,6 @@
 #include <linux/swap.h>
 #undef nr_swap_pages /* This is defined to a constant for SWAP=n case */
 
-#define VMEVENT_MAX_FREE_THRESHOD	100
-
-#define VMEVENT_MAX_EATTR_ATTRS	64
-
-struct vmevent_watch_event {
-	u64				nr_avail_pages;
-	u64				nr_free_pages;
-	u64				nr_swap_pages;
-};
-
 struct vmevent_watch {
 	struct vmevent_config		config;
Signature · 7 lines
1.7.12.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
author
#2 lore raw

PATCH 2/3 vmevent: Factor vmevent_match_attr() out of vmevent_match()

In reply to Anton Vorontsov

Soon we'll use this new function for other code; plus this makes code less indented.

Signed-off-by Anton Vorontsov <[email protected]>
---
 mm/vmevent.c | 107 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 50 deletions(-)
mm/vmevent.c +57 -50
index 39ef786..d434c11 100644
@@ -77,6 +77,59 @@ enum {
 	VMEVENT_ATTR_STATE_VALUE_WAS_GT	= (1UL << 31),
 };
 
+static bool vmevent_match_attr(struct vmevent_attr *attr, u64 value)
+{
+	u32 state = attr->state;
+	bool attr_lt = state & VMEVENT_ATTR_STATE_VALUE_LT;
+	bool attr_gt = state & VMEVENT_ATTR_STATE_VALUE_GT;
+	bool attr_eq = state & VMEVENT_ATTR_STATE_VALUE_EQ;
+	bool edge = state & VMEVENT_ATTR_STATE_EDGE_TRIGGER;
+	u32 was_lt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_LT;
+	u32 was_gt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_GT;
+	bool lt = value < attr->value;
+	bool gt = value > attr->value;
+	bool eq = value == attr->value;
+	bool was_lt = state & was_lt_mask;
+	bool was_gt = state & was_gt_mask;
+	bool was_eq = was_lt && was_gt;
+	bool ret = false;
+
+	if (!state)
+		return false;
+
+	if (!attr_lt && !attr_gt && !attr_eq)
+		return false;
+
+	if (((attr_lt && lt) || (attr_gt && gt) || (attr_eq && eq)) && !edge)
+		return true;
+
+	if (attr_eq && eq && was_eq) {
+		return false;
+	} else if (attr_lt && lt && was_lt && !was_eq) {
+		return false;
+	} else if (attr_gt && gt && was_gt && !was_eq) {
+		return false;
+	} else if (eq) {
+		state |= was_lt_mask;
+		state |= was_gt_mask;
+		if (attr_eq)
+			ret = true;
+	} else if (lt) {
+		state |= was_lt_mask;
+		state &= ~was_gt_mask;
+		if (attr_lt)
+			ret = true;
+	} else if (gt) {
+		state |= was_gt_mask;
+		state &= ~was_lt_mask;
+		if (attr_gt)
+			ret = true;
+	}
+
+	attr->state = state;
+	return ret;
+}
+
 static bool vmevent_match(struct vmevent_watch *watch)
 {
 	struct vmevent_config *config = &watch->config;

@@ -84,57 +137,11 @@ static bool vmevent_match(struct vmevent_watch *watch)
 
 	for (i = 0; i < config->counter; i++) {
 		struct vmevent_attr *attr = &config->attrs[i];
-		u32 state = attr->state;
-		bool attr_lt = state & VMEVENT_ATTR_STATE_VALUE_LT;
-		bool attr_gt = state & VMEVENT_ATTR_STATE_VALUE_GT;
-		bool attr_eq = state & VMEVENT_ATTR_STATE_VALUE_EQ;
-
-		if (!state)
-			continue;
+		u64 val;
 
-		if (attr_lt || attr_gt || attr_eq) {
-			bool edge = state & VMEVENT_ATTR_STATE_EDGE_TRIGGER;
-			u32 was_lt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_LT;
-			u32 was_gt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_GT;
-			u64 value = vmevent_sample_attr(watch, attr);
-			bool lt = value < attr->value;
-			bool gt = value > attr->value;
-			bool eq = value == attr->value;
-			bool was_lt = state & was_lt_mask;
-			bool was_gt = state & was_gt_mask;
-			bool was_eq = was_lt && was_gt;
-			bool ret = false;
-
-			if (((attr_lt && lt) || (attr_gt && gt) ||
-					(attr_eq && eq)) && !edge)
-				return true;
-
-			if (attr_eq && eq && was_eq) {
-				return false;
-			} else if (attr_lt && lt && was_lt && !was_eq) {
-				return false;
-			} else if (attr_gt && gt && was_gt && !was_eq) {
-				return false;
-			} else if (eq) {
-				state |= was_lt_mask;
-				state |= was_gt_mask;
-				if (attr_eq)
-					ret = true;
-			} else if (lt) {
-				state |= was_lt_mask;
-				state &= ~was_gt_mask;
-				if (attr_lt)
-					ret = true;
-			} else if (gt) {
-				state |= was_gt_mask;
-				state &= ~was_lt_mask;
-				if (attr_gt)
-					ret = true;
-			}
-
-			attr->state = state;
-			return ret;
-		}
+		val = vmevent_sample_attr(watch, attr);
+		if (vmevent_match_attr(attr, val))
+			return true;
 	}
 
 	return false;
Signature · 7 lines
1.7.12.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#3 lore raw

Re: [PATCH 2/3] vmevent: Factor vmevent_match_attr() out of vmevent_match()

Quoted 20 lines · Anton Vorontsov

Soon we'll use this new function for other code; plus this makes code less indented.

Signed-off-by Anton Vorontsov <[email protected]>
---
 mm/vmevent.c | 107 +++++++++++++++++++++++++++++++----------------------------
 1 file changed, 57 insertions(+), 50 deletions(-)
mm/vmevent.c +4 -0
index 39ef786..d434c11 100644
@@ -77,6 +77,59 @@ enum {
        VMEVENT_ATTR_STATE_VALUE_WAS_GT = (1UL << 31),
 };
 
+static bool vmevent_match_attr(struct vmevent_attr *attr, u64 value)
+{
+       u32 state = attr->state;
+       bool attr_lt = state & VMEVENT_ATTR_STATE_VALUE_LT;

mm/vmevent.c enum { · line 93

Quoted 10 lines
mm/vmevent.c +10 -0
+       bool attr_gt = state & VMEVENT_ATTR_STATE_VALUE_GT;
+       bool attr_eq = state & VMEVENT_ATTR_STATE_VALUE_EQ;
+       bool edge = state & VMEVENT_ATTR_STATE_EDGE_TRIGGER;
+       u32 was_lt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_LT;
+       u32 was_gt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_GT;
+       bool lt = value < attr->value;
+       bool gt = value > attr->value;
+       bool eq = value == attr->value;
+       bool was_lt = state & was_lt_mask;
+       bool was_gt = state & was_gt_mask;

[snip]

So I merged this patch but vmevent_match_attr() is still too ugly for words. It really could use some serious cleanups.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#4 lore raw

Re: [PATCH 2/3] vmevent: Factor vmevent_match_attr() out of vmevent_match()

On Fri, Oct 12, 2012 at 03:37:43PM +0300, Pekka Enberg wrote:
[...]

Quoted 17 lines
Quoted 14 lines
mm/vmevent.c +14 -0
+static bool vmevent_match_attr(struct vmevent_attr *attr, u64 value)
+{
+       u32 state = attr->state;
+       bool attr_lt = state & VMEVENT_ATTR_STATE_VALUE_LT;
+       bool attr_gt = state & VMEVENT_ATTR_STATE_VALUE_GT;
+       bool attr_eq = state & VMEVENT_ATTR_STATE_VALUE_EQ;
+       bool edge = state & VMEVENT_ATTR_STATE_EDGE_TRIGGER;
+       u32 was_lt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_LT;
+       u32 was_gt_mask = VMEVENT_ATTR_STATE_VALUE_WAS_GT;
+       bool lt = value < attr->value;
+       bool gt = value > attr->value;
+       bool eq = value == attr->value;
+       bool was_lt = state & was_lt_mask;
+       bool was_gt = state & was_gt_mask;

[snip]

Quoted 2 lines

So I merged this patch but vmevent_match_attr() is still too ugly for words. It really could use some serious cleanups.

Thanks a lot for merging these cleanups!

Yes, the patch wasn't meant to simplify the matching logic, but just to let us use the function in other places.

I once started converting the function into table-based approach, but the code started growing, and I abandoned the idea for now. I might resume the work just for the fun of it, but the code will be larger than this ad-hoc function, althouh surely it will be more generic and understandable.

But let's solve primary problems with the vmevent first. :-)

Thanks,
Anton.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw

PATCH 3/3 vmevent: Don't sample values twice

In reply to Anton Vorontsov

Currently, we sample the same values in vmevent_sample() and vmevent_match(), but we can easily avoid this. Also saves loop iterations.

Signed-off-by Anton Vorontsov <[email protected]>
---
 mm/vmevent.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)
mm/vmevent.c +7 -12
index d434c11..d643615 100644
@@ -133,18 +133,22 @@ static bool vmevent_match_attr(struct vmevent_attr *attr, u64 value)
 static bool vmevent_match(struct vmevent_watch *watch)
 {
 	struct vmevent_config *config = &watch->config;
+	bool ret = 0;
 	int i;
 
 	for (i = 0; i < config->counter; i++) {
 		struct vmevent_attr *attr = &config->attrs[i];
+		struct vmevent_attr *samp = &watch->sample_attrs[i];
 		u64 val;
 
 		val = vmevent_sample_attr(watch, attr);
-		if (vmevent_match_attr(attr, val))
-			return true;
+		if (!ret && vmevent_match_attr(attr, val))
+			ret = 1;
+
+		samp->value = val;
 	}
 
-	return false;
+	return ret;
 }
 
 /*

@@ -161,20 +165,11 @@ static bool vmevent_match(struct vmevent_watch *watch)
  */
 static void vmevent_sample(struct vmevent_watch *watch)
 {
-	int i;
-
 	if (atomic_read(&watch->pending))
 		return;
 	if (!vmevent_match(watch))
 		return;
 
-	for (i = 0; i < watch->nr_attrs; i++) {
-		struct vmevent_attr *attr = &watch->sample_attrs[i];
-
-		attr->value = vmevent_sample_attr(watch,
-						  watch->config_attrs[i]);
-	}
-
 	atomic_set(&watch->pending, 1);
 }
Signature · 7 lines
1.7.12.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Applied
#6 lore raw
In reply to Anton Vorontsov

Hi Anton,

Quoted 7 lines · Anton Vorontsov

Hello Pekka,

Just a few updates to vmevents:

- Some cleanups and refactorings -- needed for easier integration of 'memory pressure' work;
- Forward to newer Linus' tree, fix conflicts.

Applied, thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>