[PATCH 1/4] list: introduce list_is_first()

[PATCH 1/4] list: introduce list_is_first()

[Geliang Tang]

We already have list_is_last(), it makes sense to also add
list_is_first() for consistency. This list utility function
to check for first element in a list.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 include/linux/list.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/list.h b/include/linux/list.h
index 5356f4d..2c43ef4 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -170,6 +170,17 @@ static inline void list_move_tail(struct list_head *list,
 }
 
 /**
+ * list_is_first - tests whether @list is the first entry in list @head
+ * @list: the entry to test
+ * @head: the head of the list
+ */
+static inline int list_is_first(const struct list_head *list,
+				const struct list_head *head)
+{
+	return list->prev == head;
+}
+
+/**
  * list_is_last - tests whether @list is the last entry in list @head
  * @list: the entry to test
  * @head: the head of the list
-- 
2.5.0

[PATCH 2/4] livepatch: use list_is_first()

[Geliang Tang]

For better readability, use list_is_first() instead of open-coded.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 kernel/livepatch/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index bc2c85c..be64106 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -479,7 +479,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
 		return -EINVAL;
 
 	/* enforce stacking: only the first disabled patch can be enabled */
-	if (patch->list.prev != &klp_patches &&
+	if (!list_is_first(&patch->list, &klp_patches) &&
 	    list_prev_entry(patch, list)->state == KLP_DISABLED)
 		return -EBUSY;
 
-- 
2.5.0

[PATCH 3/4] netfilter: ipset: use list_is_first()

[Geliang Tang]

For better readability, use list_is_first() instead of open-coded.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 net/netfilter/ipset/ip_set_list_set.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c
index bbede95..9d757d6 100644
--- a/net/netfilter/ipset/ip_set_list_set.c
+++ b/net/netfilter/ipset/ip_set_list_set.c
@@ -288,7 +288,7 @@ list_set_uadd(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 			n = list_next_entry(next, list);
 	} else {
 		/* Insert before prev element */
-		if (prev->list.prev != &map->members)
+		if (!list_is_first(&prev->list, &map->members))
 			n = list_prev_entry(prev, list);
 	}
 	/* Can we replace a timed out entry? */
-- 
2.5.0

[PATCH 4/4] elevator: use list_is_{first,last}

[Geliang Tang]

For better readability, use list_is_{first,last}() instead of open-coded.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 block/noop-iosched.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/noop-iosched.c b/block/noop-iosched.c
index a163c48..d44326e 100644
--- a/block/noop-iosched.c
+++ b/block/noop-iosched.c
@@ -44,7 +44,7 @@ noop_former_request(struct request_queue *q, struct request *rq)
 {
 	struct noop_data *nd = q->elevator->elevator_data;
 
-	if (rq->queuelist.prev == &nd->queue)
+	if (list_is_first(&rq->queuelist, &nd->queue))
 		return NULL;
 	return list_prev_entry(rq, queuelist);
 }
@@ -54,7 +54,7 @@ noop_latter_request(struct request_queue *q, struct request *rq)
 {
 	struct noop_data *nd = q->elevator->elevator_data;
 
-	if (rq->queuelist.next == &nd->queue)
+	if (list_is_last(&rq->queuelist, &nd->queue))
 		return NULL;
 	return list_next_entry(rq, queuelist);
 }
-- 
2.5.0

Re: [PATCH 1/4] list: introduce list_is_first()

[Jens Axboe]

On 12/10/2015 07:17 AM, Geliang Tang wrote:
> We already have list_is_last(), it makes sense to also add
> list_is_first() for consistency. This list utility function
> to check for first element in a list.

Honestly, I think we already have way too many of these kind of helpers. 
IMHO they don't really help, they hurt readability. You should know how 
the list works anyway, and if you do, then it's a no-brainer what's 
first and last. If you don't, then you are bound to screw up in other ways.

Just my 2 cents.

-- 
Jens Axboe

Re: [PATCH 1/4] list: introduce list_is_first()

[Josh Poimboeuf]

On Thu, Dec 10, 2015 at 08:10:34AM -0700, Jens Axboe wrote:
> On 12/10/2015 07:17 AM, Geliang Tang wrote:
> >We already have list_is_last(), it makes sense to also add
> >list_is_first() for consistency. This list utility function
> >to check for first element in a list.
> 
> Honestly, I think we already have way too many of these kind of helpers.
> IMHO they don't really help, they hurt readability. You should know how the
> list works anyway, and if you do, then it's a no-brainer what's first and
> last. If you don't, then you are bound to screw up in other ways.
> 
> Just my 2 cents.

Personally I would disagree.  Something like:

  if (list_is_first(&rq->queuelist, &nd->queue))

is much more readable to me than:

  if (rq->queuelist.prev == &nd->queue)

The first one takes no effort for me -- it's almost English.  While the
second one takes me a few seconds (and some precious brain cycles) to
decipher.

Maybe whether it's readable depends on how many years you've been
looking at the pattern.  But IMHO we shouldn't make "having x # of years
staring at kernel code" a prerequisite for being able to read kernel
code.

-- 
Josh

Re: [PATCH 1/4] list: introduce list_is_first()

[Jens Axboe]

On 12/10/2015 08:23 AM, Josh Poimboeuf wrote:
> On Thu, Dec 10, 2015 at 08:10:34AM -0700, Jens Axboe wrote:
>> On 12/10/2015 07:17 AM, Geliang Tang wrote:
>>> We already have list_is_last(), it makes sense to also add
>>> list_is_first() for consistency. This list utility function
>>> to check for first element in a list.
>>
>> Honestly, I think we already have way too many of these kind of helpers.
>> IMHO they don't really help, they hurt readability. You should know how the
>> list works anyway, and if you do, then it's a no-brainer what's first and
>> last. If you don't, then you are bound to screw up in other ways.
>>
>> Just my 2 cents.
>
> Personally I would disagree.  Something like:
>
>    if (list_is_first(&rq->queuelist, &nd->queue))
>
> is much more readable to me than:
>
>    if (rq->queuelist.prev == &nd->queue)

Both the function and your example are backwards, and hence a lot harder 
to comprehend than they should be. It'd be much clearer as:

     if (nd->queue.next == &rq->queuelist)

which is a lot easier to read. Nobody should open-code a 'is this the 
first entry in the list' by asking 'is the previous link to my node the 
head', asking 'is the next entry in the list X' makes a lot more sense. 
I'm assuming this happened because the list_is_last was just copied and 
modified, instead of thinking about this for a second.

> The first one takes no effort for me -- it's almost English.  While the
> second one takes me a few seconds (and some precious brain cycles) to
> decipher.
>
> Maybe whether it's readable depends on how many years you've been
> looking at the pattern.  But IMHO we shouldn't make "having x # of years
> staring at kernel code" a prerequisite for being able to read kernel
> code.

It's a balance, as we also should not make APIs out of everything. As I 
said, purely my opinion, but I think the is_last/is_first have jumped 
the shark.

-- 
Jens Axboe

Re: [PATCH 1/4] list: introduce list_is_first()

[Christoph Hellwig]

On Thu, Dec 10, 2015 at 09:23:57AM -0600, Josh Poimboeuf wrote:
> Personally I would disagree.  Something like:
> 
>   if (list_is_first(&rq->queuelist, &nd->queue))
> 
> is much more readable to me than:
> 
>   if (rq->queuelist.prev == &nd->queue)
> 
> The first one takes no effort for me -- it's almost English.  While the
> second one takes me a few seconds (and some precious brain cycles) to
> decipher.
> 
> Maybe whether it's readable depends on how many years you've been
> looking at the pattern.  But IMHO we shouldn't make "having x # of years
> staring at kernel code" a prerequisite for being able to read kernel
> code.

I think understanding the list.h semantics is a requirement for writing
(or reading) non-trivial kernel code.

Re: [PATCH 1/4] list: introduce list_is_first()

[Jiri Kosina]

On Thu, 10 Dec 2015, Jens Axboe wrote:

> It's a balance, as we also should not make APIs out of everything. As I said,
> purely my opinion, but I think the is_last/is_first have jumped the shark.

I don't have a strong opinion either way.

What I think we should do though, is to either have both (i.e accept this 
patchset) or have neither of them (i.e. drop list_is_last()).

Otherwise people are likely to be confused by such an asymetric API and 
will keep posting patches for it over and over again.

-- 
Jiri Kosina
SUSE Labs