[PATCH] drivers/block/ub.c: use list_for_each_entry()

[PATCH] drivers/block/ub.c: use list_for_each_entry()

[Matthias Kaehlcke]

Low performance USB storage driver: Use list_for_each_entry() instead
of list_for_each()

Signed-off-by: Matthias Kaehlcke <matthias.kaehlcke@gmail.com>

--

diff --git a/drivers/block/ub.c b/drivers/block/ub.c
index 746a118..18c8b6c 100644
--- a/drivers/block/ub.c
+++ b/drivers/block/ub.c
@@ -1547,10 +1547,8 @@ static void ub_reset_enter(struct ub_dev *sc, int try)
 #endif
 
 #if 0 /* We let them stop themselves. */
-	struct list_head *p;
 	struct ub_lun *lun;
-	list_for_each(p, &sc->luns) {
-		lun = list_entry(p, struct ub_lun, link);
+	list_for_each_entry(lun, &sc->luns, link) {
 		blk_stop_queue(lun->disk->queue);
 	}
 #endif
@@ -1562,7 +1560,6 @@ static void ub_reset_task(struct work_struct *work)
 {
 	struct ub_dev *sc = container_of(work, struct ub_dev, reset_work);
 	unsigned long flags;
-	struct list_head *p;
 	struct ub_lun *lun;
 	int lkr, rc;
 
@@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
 	spin_lock_irqsave(sc->lock, flags);
 	sc->reset = 0;
 	tasklet_schedule(&sc->tasklet);
-	list_for_each(p, &sc->luns) {
-		lun = list_entry(p, struct ub_lun, link);
+	list_for_each_entry(lun, &sc->luns, link) {
 		blk_start_queue(lun->disk->queue);
 	}
 	wake_up(&sc->reset_wait);
@@ -2348,7 +2344,6 @@ err_alloc:
 static void ub_disconnect(struct usb_interface *intf)
 {
 	struct ub_dev *sc = usb_get_intfdata(intf);
-	struct list_head *p;
 	struct ub_lun *lun;
 	unsigned long flags;
 
@@ -2403,8 +2398,7 @@ static void ub_disconnect(struct usb_interface *intf)
 	/*
 	 * Unregister the upper layer.
 	 */
-	list_for_each (p, &sc->luns) {
-		lun = list_entry(p, struct ub_lun, link);
+	list_for_each_entry(lun, &sc->luns, link) {
 		del_gendisk(lun->disk);
 		/*
 		 * I wish I could do:

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

             "The only important thing Windows does better
            than Debian is implementing the win32 platform"
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Pete Zaitcev]

On Wed, 30 May 2007 10:47:52 +0200, Matthias Kaehlcke <matthias.kaehlcke@gmail.com> wrote:

> @@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
>  	spin_lock_irqsave(sc->lock, flags);
>  	sc->reset = 0;
>  	tasklet_schedule(&sc->tasklet);
> -	list_for_each(p, &sc->luns) {
> -		lun = list_entry(p, struct ub_lun, link);
> +	list_for_each_entry(lun, &sc->luns, link) {
>  		blk_start_queue(lun->disk->queue);
>  	}
>  	wake_up(&sc->reset_wait);

This patch straddles the border of acceptable. The pointless obfuscation
is balanced against the removal of explicit type in list_entry() and thus
a possible mismatched struct. I'm not acking nor naking this.

-- Pete

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Matthias Kaehlcke]

El Wed, May 30, 2007 at 12:38:40PM -0700 Pete Zaitcev ha dit:

> On Wed, 30 May 2007 10:47:52 +0200, Matthias Kaehlcke <matthias.kaehlcke@gmail.com> wrote:
> 
> > @@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
> >  	spin_lock_irqsave(sc->lock, flags);
> >  	sc->reset = 0;
> >  	tasklet_schedule(&sc->tasklet);
> > -	list_for_each(p, &sc->luns) {
> > -		lun = list_entry(p, struct ub_lun, link);
> > +	list_for_each_entry(lun, &sc->luns, link) {
> >  		blk_start_queue(lun->disk->queue);
> >  	}
> >  	wake_up(&sc->reset_wait);
> 
> This patch straddles the border of acceptable. The pointless obfuscation
> is balanced against the removal of explicit type in list_entry() and thus
> a possible mismatched struct. I'm not acking nor naking this.

if i understand you correctly the problem isn't so much the patch, but
the use of list_for_each_entry() in general. i thought
list_for_each_entry() is preferred over list_for_each() when its use
is possible. 

i understand your point, though i think only a chain of errors would
make list_for_each_entry() a problem without being notified by the
compiler:

1) the mismatched struct must have a list_head pointer
2) the name of this list_head pointer must match the name in
list_for_each_entry()
3) the mismatched struct must be 'compatible' with the code in the
loop

please correct me if i misinterpreted the reason of your concerns

regards

-- 
Matthias Kaehlcke
Linux Application Developer
Barcelona

      El trabajo es el refugio de los que no tienen nada que hacer
                            (Oscar Wilde)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Satyam Sharma]

Hi Pete,

On 5/31/07, Pete Zaitcev <zaitcev@redhat.com> wrote:
> On Wed, 30 May 2007 10:47:52 +0200, Matthias Kaehlcke <matthias.kaehlcke@gmail.com> wrote:
>
> > @@ -1608,8 +1605,7 @@ static void ub_reset_task(struct work_struct *work)
> >       spin_lock_irqsave(sc->lock, flags);
> >       sc->reset = 0;
> >       tasklet_schedule(&sc->tasklet);
> > -     list_for_each(p, &sc->luns) {
> > -             lun = list_entry(p, struct ub_lun, link);
> > +     list_for_each_entry(lun, &sc->luns, link) {
> >               blk_start_queue(lun->disk->queue);
> >       }
> >       wake_up(&sc->reset_wait);
>
> This patch straddles the border of acceptable. The pointless obfuscation
> is balanced against the removal of explicit type in list_entry() and thus
> a possible mismatched struct. I'm not acking nor naking this.

A list_for_each() followed by list_entry() ---> list_for_each_entry()
conversion is a pretty harmless (and desirable) conversion, IMO.
In fact list_for_each_entry() itself uses list_entry(..., typeof(*p), ...)
which seems to be a safer way to use list_entry() than specifying
the type explicity/manually in its arguments, no?

Satyam

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Pete Zaitcev]

On Thu, 31 May 2007 02:44:17 +0530, "Satyam Sharma" <satyam.sharma@gmail.com> wrote:

> > > -     list_for_each(p, &sc->luns) {
> > > -             lun = list_entry(p, struct ub_lun, link);
> > > +     list_for_each_entry(lun, &sc->luns, link) {

> > This patch straddles the border of acceptable. The pointless obfuscation
> > is balanced against the removal of explicit type in list_entry() and thus
> > a possible mismatched struct. I'm not acking nor naking this.
> 
> A list_for_each() followed by list_entry() ---> list_for_each_entry()
> conversion is a pretty harmless (and desirable) conversion, IMO.
> In fact list_for_each_entry() itself uses list_entry(..., typeof(*p), ...)
> which seems to be a safer way to use list_entry() than specifying
> the type explicity/manually in its arguments, no?

I believe I have mentioned the type safety as a positive, and in fact
it's quoted above. I just didn't think it necessary to use quite as
many words explaining it until now.

The negative is the sheer number of helper functions in list.h. Personally,
I find it difficult to retain a working knowledge of them. Iterators are
particularly nasty that way. I'm thinking about dropping all of these
list_for_each_with_murky_argument_requirements_and_odd_side_effects()
and use plain for(;;), as a courtesy to someone who has to read the
code years down the road.

-- Pete

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Roland Dreier]

 > The negative is the sheer number of helper functions in list.h. Personally,
 > I find it difficult to retain a working knowledge of them. Iterators are
 > particularly nasty that way. I'm thinking about dropping all of these
 > list_for_each_with_murky_argument_requirements_and_odd_side_effects()
 > and use plain for(;;), as a courtesy to someone who has to read the
 > code years down the road.

I think I disagree with this reasoning.  If I'm reading your code and
I see, say, list_for_each_entry_safe(), I can be pretty confident that
your loop works correctly.  If you write your own for loop, then I
have to check that you actually got the linked list walking right.

 - R.

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Pete Zaitcev]

On Wed, 30 May 2007 16:14:01 -0700, Roland Dreier <rdreier@cisco.com> wrote:

>  > The negative is the sheer number of helper functions in list.h. Personally,
>  > I find it difficult to retain a working knowledge of them. Iterators are
>  > particularly nasty that way. I'm thinking about dropping all of these
>  > list_for_each_with_murky_argument_requirements_and_odd_side_effects()
>  > and use plain for(;;), as a courtesy to someone who has to read the
>  > code years down the road.
> 
> I think I disagree with this reasoning.  If I'm reading your code and
> I see, say, list_for_each_entry_safe(), I can be pretty confident that
> your loop works correctly.  If you write your own for loop, then I
> have to check that you actually got the linked list walking right.

You have to check that I used list_for_each_entry_safe correctly too,
which is harder. Are you aware that we had (and probably still have)
dozens of cases where the use of list_for_each_entry_safe was buggy?
Most of them involved IHV programmers being lured into false sense
of security by the _safe suffix and getting their locking wrong.

You could not find a better way to blow up your own argument
than to mention list_for_each_entry_safe(), which is anything but.
Matthias' use of list_for_each_entry() actually IS safe, which is
why I am not NAKing it. Andrew has accepted it already. I just
think we aren't winning squat here.

-- Pete

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Roland Dreier]

 > > > The negative is the sheer number of helper functions in list.h. Personally,
 > > > I find it difficult to retain a working knowledge of them. Iterators are
 > > > particularly nasty that way. I'm thinking about dropping all of these
 > > > list_for_each_with_murky_argument_requirements_and_odd_side_effects()
 > > > and use plain for(;;), as a courtesy to someone who has to read the
 > > > code years down the road.
 > > 
 > > I think I disagree with this reasoning.  If I'm reading your code and
 > > I see, say, list_for_each_entry_safe(), I can be pretty confident that
 > > your loop works correctly.  If you write your own for loop, then I
 > > have to check that you actually got the linked list walking right.
 > 
 > You have to check that I used list_for_each_entry_safe correctly too,
 > which is harder. Are you aware that we had (and probably still have)
 > dozens of cases where the use of list_for_each_entry_safe was buggy?
 > Most of them involved IHV programmers being lured into false sense
 > of security by the _safe suffix and getting their locking wrong.
 > 
 > You could not find a better way to blow up your own argument
 > than to mention list_for_each_entry_safe(), which is anything but.
 > Matthias' use of list_for_each_entry() actually IS safe, which is
 > why I am not NAKing it. Andrew has accepted it already. I just
 > think we aren't winning squat here.

Well, actually, I chose list_for_each_entry_safe() quite conscious of
the locking issues.  If I see list_XXX_safe() then I know that I need
to be suspicious when reviewing code -- the same way seeing "atomic_t"
makes me think "is there any reason to use atomic_t??"

If I just see

	for (pos = list_entry((head)->next, typeof(*pos), member),
		n = list_entry(pos->member.next, typeof(*pos), member);
	     &pos->member != (head);
	     pos = n, n = list_entry(n->member.next, typeof(*n), member))

then what am I to think?

 - R.

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Pete Zaitcev]

On Wed, 30 May 2007 16:42:41 -0700, Roland Dreier <rdreier@cisco.com> wrote:

> If I just see
> 
> 	for (pos = list_entry((head)->next, typeof(*pos), member),
> 		n = list_entry(pos->member.next, typeof(*pos), member);
> 	     &pos->member != (head);
> 	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
> 
> then what am I to think?

You won't catch me writing this kind of crap, so the question is moot.
Seriously, a comma operator? Admit it, you just expanded a marcro from
list.h by hand. Real people cannot write like that.

-- Pete

Re: [PATCH] drivers/block/ub.c: use list_for_each_entry()

[Roland Dreier]

 > > If I just see
 > > 
 > > 	for (pos = list_entry((head)->next, typeof(*pos), member),
 > > 		n = list_entry(pos->member.next, typeof(*pos), member);
 > > 	     &pos->member != (head);
 > > 	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
 > > 
 > > then what am I to think?
 > 
 > You won't catch me writing this kind of crap, so the question is moot.
 > Seriously, a comma operator? Admit it, you just expanded a marcro from
 > list.h by hand. Real people cannot write like that.

Of course I admit it, that is a copy of the definition of list_for_each_safe()
(with just the '/'s removed).  But the point is, if you are writing
something that iterates through a list and deletes entries, you
basically have to write equivalent code.

Just think about how many silly bugs you've written in your life when
(re)implementing linked lists.  By using <linux/list.h>, you avoid all
that, and as a code reviewer that makes my life easier.  It's the same
theory as <linux/kref.h> -- the code is rather trivial (although as
"git log lib/kref.c" shows, not entirely trivial).  But if I see
someone using struct kref, all I have to check is whether she used it
correctly.  I don't have to worry about whether she screwed up the
implementation.

 - R.