Low performance USB storage driver: Use list_for_each_entry() instead
of list_for_each()
Signed-off-by: Matthias Kaehlcke <[email protected]>
--
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 `-
On Wed, 30 May 2007 10:47:52 +0200, Matthias Kaehlcke <[email protected]> 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
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 <[email protected]> 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 `-
Hi Pete,
On 5/31/07, Pete Zaitcev <[email protected]> wrote:
> On Wed, 30 May 2007 10:47:52 +0200, Matthias Kaehlcke <[email protected]> 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
On Thu, 31 May 2007 02:44:17 +0530, "Satyam Sharma" <[email protected]> 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
> 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.
On Wed, 30 May 2007 16:14:01 -0700, Roland Dreier <[email protected]> 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
> > > 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.
On Wed, 30 May 2007 16:42:41 -0700, Roland Dreier <[email protected]> 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
> > 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.