Skip to contentSkip to conversation

sort: Introduce generic list_sort function

PATCH In review Andi Kleen 3 participants 5 messages Jan 5, 2010 – Jan 6, 2010 1 message not in the archive

Subsystems (4)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 1262649295-28427-1-git-send-email-david__25057.2445955642$1262651404$gmane$org@fromorbit.com

Andi Kleen

author
#1 lore raw
Quoted 8 lines · Dave Chinner
(quoted diff) +8 -0
+
+/**
+ * list_sort - sort a list.
+ * @priv: private data, passed to @cmp
+ * @head: the list to sort
+ * @cmp: the elements comparison function
+ *
+ * This function has been implemented by Mark J Roberts <[email protected]>. It

Review

Quoted 10 lines
(quoted diff) +10 -0
+ * implements "merge sort" which has O(nlog(n)) complexity. The list is sorted
+ * in ascending order.
+ *
+ * The comparison function @cmp is supposed to return a negative value if @a is
+ * than @b, and a positive value if @a is greater than @b. If @a and @b are
+ * equivalent, then it does not matter what this function returns.
+ */
+void list_sort(void *priv, struct list_head *head,
+	       int (*cmp)(void *priv, struct list_head *a,
+			  struct list_head *b))

No EXPORT_SYMBOL?

Also it would seem cleaner to have it in a own file.

-Andi

Signature · 6 lines
[email protected] -- Speaking for myself only.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#2 lore raw

PATCH v3 1/1 sort: Introduce generic list_sort function

Quoted 13 lines · Andi Kleen
Quoted 12 lines · Dave Chinner
(quoted diff) +12 -0
+
+/**
+ * list_sort - sort a list.
+ * @priv: private data, passed to @cmp
+ * @head: the list to sort
+ * @cmp: the elements comparison function
+ *
+ * This function has been implemented by Mark J Roberts <[email protected]>. It
+ * implements "merge sort" which has O(nlog(n)) complexity. The list is sorted
+ * in ascending order.
+ *
+ * The comparison function @cmp is supposed to return a negative value if @a is
Quoted 8 lines

Review

Quoted 6 lines

+ * than @b, and a positive value if @a is greater than @b. If @a and @b are
+ * equivalent, then it does not matter what this function returns.
+ */

(quoted diff) +3 -0
+void list_sort(void *priv, struct list_head *head,
+	       int (*cmp)(void *priv, struct list_head *a,
+			  struct list_head *b))

No EXPORT_SYMBOL?

Bah, who uses modules? :P

Fixed. Updated patch below.

Quoted 1 line

Also it would seem cleaner to have it in a own file.

That might make sense if we had a large number of generic sort functions and it was difficult to tell the code apart, but we've only got 2 right now....

Cheers,

Dave.

Signature · 22 lines
Dave Chinner
[email protected]

sort: Introduce generic list_sort function

There are two copies of list_sort() in the tree already, one in the DRM code,
another in ubifs. Now XFS needs this as well. Create a generic list_sort()
function from the ubifs version and convert existing users to it so we
don't end up with yet another copy in the tree.

Signed-off-by: Dave Chinner <[email protected]>
Acked-by: Dave Airlie <[email protected]>
Acked-by: Artem Bityutskiy <[email protected]>
---
V3: Export list_sort()
V2: fix a typo

 drivers/gpu/drm/drm_modes.c |   90 ++-------------------------------------
 fs/ubifs/gc.c               |   95 -----------------------------------------
 include/linux/sort.h        |    5 ++
 lib/sort.c                  |   98 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 181 deletions(-)
drivers/gpu/drm/drm_modes.c +4 -86
index 51f6772..3846ed4 100644
@@ -1,9 +1,4 @@
 /*
- * The list_sort function is (presumably) licensed under the GPL (see the
- * top level "COPYING" file for details).
- *
- * The remainder of this file is:
- *
  * Copyright © 1997-2003 by The XFree86 Project, Inc.
  * Copyright © 2007 Dave Airlie
  * Copyright © 2007-2008 Intel Corporation
@@ -36,6 +31,7 @@
  */
 
 #include <linux/list.h>
+#include <linux/sort.h>
 #include "drmP.h"
 #include "drm.h"
 #include "drm_crtc.h"
@@ -829,6 +825,7 @@ EXPORT_SYMBOL(drm_mode_prune_invalid);
 
 /**
  * drm_mode_compare - compare modes for favorability
+ * @priv: unused
  * @lh_a: list_head for first mode
  * @lh_b: list_head for second mode
  *
@@ -842,7 +839,7 @@ EXPORT_SYMBOL(drm_mode_prune_invalid);
  * Negative if @lh_a is better than @lh_b, zero if they're equivalent, or
  * positive if @lh_b is better than @lh_a.
  */
-static int drm_mode_compare(struct list_head *lh_a, struct list_head *lh_b)
+static int drm_mode_compare(void *priv, struct list_head *lh_a, struct list_head *lh_b)
 {
 	struct drm_display_mode *a = list_entry(lh_a, struct drm_display_mode, head);
 	struct drm_display_mode *b = list_entry(lh_b, struct drm_display_mode, head);
@@ -859,85 +856,6 @@ static int drm_mode_compare(struct list_head *lh_a, struct list_head *lh_b)
 	return diff;
 }
 
-/* FIXME: what we don't have a list sort function? */
-/* list sort from Mark J Roberts ([email protected]) */
-void list_sort(struct list_head *head,
-	       int (*cmp)(struct list_head *a, struct list_head *b))
-{
-	struct list_head *p, *q, *e, *list, *tail, *oldhead;
-	int insize, nmerges, psize, qsize, i;
-
-	list = head->next;
-	list_del(head);
-	insize = 1;
-	for (;;) {
-		p = oldhead = list;
-		list = tail = NULL;
-		nmerges = 0;
-
-		while (p) {
-			nmerges++;
-			q = p;
-			psize = 0;
-			for (i = 0; i < insize; i++) {
-				psize++;
-				q = q->next == oldhead ? NULL : q->next;
-				if (!q)
-					break;
-			}
-
-			qsize = insize;
-			while (psize > 0 || (qsize > 0 && q)) {
-				if (!psize) {
-					e = q;
-					q = q->next;
-					qsize--;
-					if (q == oldhead)
-						q = NULL;
-				} else if (!qsize || !q) {
-					e = p;
-					p = p->next;
-					psize--;
-					if (p == oldhead)
-						p = NULL;
-				} else if (cmp(p, q) <= 0) {
-					e = p;
-					p = p->next;
-					psize--;
-					if (p == oldhead)
-						p = NULL;
-				} else {
-					e = q;
-					q = q->next;
-					qsize--;
-					if (q == oldhead)
-						q = NULL;
-				}
-				if (tail)
-					tail->next = e;
-				else
-					list = e;
-				e->prev = tail;
-				tail = e;
-			}
-			p = q;
-		}
-
-		tail->next = list;
-		list->prev = tail;
-
-		if (nmerges <= 1)
-			break;
-
-		insize *= 2;
-	}
-
-	head->next = list;
-	head->prev = list->prev;
-	list->prev->next = head;
-	list->prev = head;
-}
-
 /**
  * drm_mode_sort - sort mode list
  * @mode_list: list to sort
@@ -949,7 +867,7 @@ void list_sort(struct list_head *head,
  */
 void drm_mode_sort(struct list_head *mode_list)
 {
-	list_sort(mode_list, drm_mode_compare);
+	list_sort(NULL, mode_list, drm_mode_compare);
 }
 EXPORT_SYMBOL(drm_mode_sort);
 
fs/ubifs/gc.c +0 -95

101 lines not on this page · raw message

include/linux/sort.h +5 -0
index d534da2..99a2ed5 100644
@@ -3,8 +3,13 @@
 
 #include <linux/types.h>
 
+struct list_head;
+
 void sort(void *base, size_t num, size_t size,
 	  int (*cmp)(const void *, const void *),
 	  void (*swap)(void *, void *, int));
 
+void list_sort(void *priv, struct list_head *head,
+	       int (*cmp)(void *priv, struct list_head *a,
+			  struct list_head *b));
 #endif
lib/sort.c +98 -0

107 lines not on this page · raw message

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#3 lore raw
Quoted 5 lines
Quoted 1 line

Also it would seem cleaner to have it in a own file.

That might make sense if we had a large number of generic sort functions and it was difficult to tell the code apart, but we've only got 2 right now....

I was more thinking of the case that it can be easily made a lib-y and then eliminated by the linker on non modular kernels if not needed (unfortunately that would require putting the EXPORT_SYMBOL somewhere else)

-Andi

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#4 lore raw
Quoted 9 lines · Andi Kleen
Quoted 5 lines
Quoted 1 line

Also it would seem cleaner to have it in a own file.

That might make sense if we had a large number of generic sort functions and it was difficult to tell the code apart, but we've only got 2 right now....

I was more thinking of the case that it can be easily made a lib-y and then eliminated by the linker on non modular kernels if not needed (unfortunately that would require putting the EXPORT_SYMBOL somewhere else)

lib-y doesn't work together with EXPORT_SYMBOL, having the export outside would also always pull it in. These days the whole lib-y mess doesn't make sense anymore - if we really need an optional library symbol we can just pull it in through a Kconfig variable.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#5 lore raw
Quoted 4 lines

lib-y doesn't work together with EXPORT_SYMBOL, having the export outside would also always pull it in. These days the whole lib-y mess doesn't make sense anymore - if we really need an optional library symbol we can just pull it in through a Kconfig variable.

It works for non modular kernels with CONFIG_MODULES turned off.

See also the other patch I posted yesterday to fix this for lib/*

-Andi

Signature · 6 lines
[email protected] -- Speaking for myself only.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs