Message-ID: <AANLkTikDymb5OGkmxZkAZwvr-uUPK64jcmxsV1sFOfEX@mail.gmail.com>
Date:	Thu, 14 Oct 2010 15:23:31 -0700
From:	Jesse Gross <jesse@...ira.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/7] vlan: Centralize handling of hardware acceleration.

On Wed, Oct 13, 2010 at 2:12 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> Le mercredi 13 octobre 2010 à 13:02 -0700, Jesse Gross a écrit :
>> +#define VLAN_N_VID           4096
>>
>
> This should be a patch on its own (change VLAN_GROUP_ARRAY_LEN to
> VLAN_N_ID), because this patch is too big.

That's fine, I separated it out.

>
> Please try to not change too many things at once, you remove many
> temporary variables and this only makes review very time consuming.

For what's worth, it used to be worse: originally all seven of these
patches were one...

>>  #if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
>> +/* Must be invoked with rcu_read_lock or with RTNL. */
>> +static inline struct net_device *vlan_find_dev(struct net_device *real_dev,
>> +                                            u16 vlan_id)
>> +{
>> +     struct vlan_group *grp = rcu_dereference(real_dev->vlgrp);
>> +
>
> This rcu_dereference() doesnt match the comment.
>
> You might want rcu_dereference_rtnl() instead and use CONFIG_PROVE_RCU

Sure, I changed it to use rcu_dereference_rtnl().

>>  static inline int netif_needs_gso(struct net_device *dev, struct sk_buff *skb)
>>  {
>> +     int features = dev->features;
>> +
>> +     if (skb->protocol == htons(ETH_P_8021Q) || skb->vlan_tci)
>> +             features &= dev->vlan_features;
>> +
>>       return skb_is_gso(skb) &&
>> -            (!skb_gso_ok(skb, dev->features) ||
>> +            (!skb_gso_ok(skb, features) ||
>>               unlikely(skb->ip_summed != CHECKSUM_PARTIAL));
>
>
> Maybe reorder tests to common case, avoiding some uneeded computations
> if !skb_is_gso()

That's a good idea, thanks.

>> -void vlan_hwaccel_do_receive(struct sk_buff *skb)
>> -{
>> -     struct net_device *dev = skb->dev;
>
> this temporary variable was nice for a better code readability

I changed all these references to use vlan_dev.

>
>> -     struct vlan_rx_stats     *rx_stats;
>> +     if (netpoll_receive_skb(skb))
>> +             return NET_RX_DROP;
>>
>> -     skb->dev = vlan_dev_real_dev(dev);
>
>>       netif_nit_deliver(skb);
> Strange you dont change netif_nit_deliver() ?

netif_nit_deliver() is used in pretty much the same manner that it was
before, which is why I didn't change it.  Specifically, it can already
handle pulling the tag out of skb->vlan_tci on the underlying device.
tcpdump works as expected after my changes.  Is there something that
you think I'm missing?

> I believe this stuff is a great idea, but you should take more time to
> make your patches more understandable.
>
> Given 2.6.36 is about to be released, and Netfilter Workshop 2010 begins
> in few days, there is no hurry, because there is no chance we add so
> many fundamental changes before three weeks at least.
>
> I believe this patch (2/7), should be split in small units, maybe 3 or 4
> different patches.

Thanks for the review Eric.  I made the above changes plus broke this
patch down into 4 separate components (and you're right, it is much
easier to look through).  I'll hold onto the series until things open
up again.  In the meantime I'll also try to convert over more of the
drivers.

Thanks again.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists