On Fri, Dec 12, 2008 at 10:25:24PM +0000, Ben Hutchings wrote:
quoted text
> On Fri, 2008-12-12 at 16:31 +1100, Herbert Xu wrote:
> > Whenever the skb is merged into an existing entry, the gro_receive
> > function should set NAPI_GRO_CB(skb)->same_flow.  Note that if an skb
> > merely matches an existing entry but can't be merged with it, then
> > this shouldn't be set.
> 
> So why not call this field "merged"?

Because when it's used on held skb's it indicates whether the new
skb can be of the same flow or not.

quoted text
> > +int napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
> > +{
> > +	struct sk_buff **pp;
> > +	struct packet_type *ptype;
> > +	__be16 type = skb->protocol;
> > +	struct list_head *head = &ptype_base[ntohs(type) & PTYPE_HASH_MASK];
> 
> Are you intending for the VLAN driver to call napi_gro_receive()?  If
> not, I think this should treat VLAN tags as part of the MAC header.
> Not every NIC separates them out!

Yes the idea is to look at the VLAN+Ethernet header in future.

quoted text
> > +		for (p = napi->gro_list; p; p = p->next) {
> > +			count++;
> > +			NAPI_GRO_CB(p)->same_flow =
> > +				p->mac_len == mac_len &&
> > +				!memcmp(skb_mac_header(p), skb_mac_header(skb),
> > +					mac_len);
> > +			NAPI_GRO_CB(p)->flush = 0;
> 
> Is this assignment to flush really necessary?  Surely any skb on the
> gro_list with flush == 1 gets removed before the next call to
> napi_gro_receive()?

Yes it is necessary.  Only the one with flushing != 0 and is of the
same flow as the new skb will be removed.

quoted text
> The inet_lro code accepts either skbs or pages and the sfc driver takes
> advantage of this: so long as most packets can be coalesced by LRO, it's
> cheaper to allocate page buffers in advance and then attach them to skbs
> during LRO.  I think you should support the use of page buffers.
> Obviously it adds complexity but there's a real performance benefit.
> (Alternately you could work out how to make skb allocation cheaper, and
> everyone would be happy!)

This is something which I will look at in future.  The top priority
right now is to get bridging to work with GRO.

quoted text
> Shouldn't the list already be empty at this point?

It should be.  However, if your driver is buggy and forgets to
call napi_complete (or calls __napi_complete without napi_gro_flush)
then we'll need this.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html