lkml.org 
[lkml]   [2004]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11
Date
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>

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:08    [W:0.052 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site