Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11

[Ed L Cashin <ecashin@coraid.com>]

Scott Feldman <sfeldma@pobox.com> writes:

...
>> +static char aoe_iflist[IFLISTSZ];
>> +
>> +int
>> +is_aoe_netif(struct net_device *ifp)
>> +{
>> +       register char *p, *q;
>> +       register int len;
>> +
>> +       if (aoe_iflist[0] == '\0')
>> +               return 1;
>> +
>> +       for (p = aoe_iflist; *p; p = q + strspn(q, WHITESPACE)) {
>> +               q = p + strcspn(p, WHITESPACE);
>> +               if (q != p)
>> +                       len = q - p;
>> +               else
>> +                       len = strlen(p); /* last token in aoe_iflist
> */
>> +
>> +               if (strlen(ifp->name) == len && !strncmp(ifp->name, p,
> len))
>> +                       return 1;
>> +               if (q == p)
>> +                       break;
>> +       }
>> +
>> +       return 0;
>> +}
>
> This is getting called for every skb received.  Do you really want to
> walk a string array looking for "eth0" or "eth1" for every receive
> packet?  Maybe making the locals "register" helps speed things up.  :-)
>
> Seriously, can you achieve the same by registering the packet type
> handler for each dev and let netif_receive_skb() do the check for
> skb->dev == packet_type->dev?

We'd like to keep state in the driver in order to be able to control
both outgoing and incoming traffic.  I'm not sure this check will
become a performance problem, but anyone with who can measure it is
welcome to speak up.  I suspect that optimizing this now would be a
case of premature optimization.

>> + * (1) i have no idea if this is redundant, but i can't figure why
>> + * the ifp is passed in if it is.
>
> This isn't necessary - see deliver_skb()

Thanks.  That looks like the only place our packet handler is called.

>> +       skb->len += ETH_HLEN;   /* (2) */
>
> You want to do a skb_push(skb, ETH_HLEN) here instead to keep skb->len
> and skb->data corresponding (basically to undo the skb->pull() in
> eth_type_trans()).  skb->mac.raw will still be correct.

Thanks.

...
>> +       if (h->verfl & AOEFL_ERR) {
>> +               n = h->err;
>> +               if (n > NECODES)
>> +                       n = 0;
>> +               printk(KERN_CRIT "aoe: aoenet_rcv: error packet from
> %d.%d; "
>> +                       "ecode=%d '%s'\n",
>> +                      __be16_to_cpu(*((u16 *) h->major)), h->minor,
>> +                       h->err, aoe_errlist[n]);
>> +               goto exit;
>> +       }
>
> Is there a better way to handle errors than flooding the log?

These errors haven't happened much, but if they did, I'd want
everybody to know.  Would you suggest a per-device counter to limit
how many times this message gets printed?  

>> +       dev_kfree_skb(skb);
>
> kfree_skb() should be enough.

Hmm.  It's just a macro for kfree_skb, but I thought that
dev_kfree_skb is the more correct one for us to call.  I mean, if
dev_kfree_skb isn't supposed to be called here, then what's it for?

-- 
  Ed L Cashin <ecashin@coraid.com>