Messages in this thread | | | From | Ed L Cashin <> | Subject | Re: [PATCH] ATA over Ethernet driver for 2.6.10-rc3-bk11 | Date | Wed, 22 Dec 2004 08:19:50 -0500 |
| |
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/
| |