Re: [PATCH] vlan: Keep NETIF_F_HW_CSUM similar to other software devices

[Vlad Yasevich <vyasevic@redhat.com>]

On 05/05/2017 04:01 PM, Alexander Duyck wrote:
> On Fri, May 5, 2017 at 12:20 PM, Vladislav Yasevich <vyasevich@gmail.com> wrote:
>> Vlan devices, like all other software devices, enable
>> NETIF_F_HW_CSUM feature.  However, unlike all the othe other
>> software devices, vlans will switch to using IP|IPV6_CSUM
>> features, if the underlying devices uses them.  In these situations,
>> checksum offload features on the vlan device can't be controlled
>> via ethtool.
>>
>> This patch makes vlans keep HW_CSUM feature if the underlying
>> device supports checksum offloading.  This makes vlan devices
>> behave like other software devices, and restores control to the
>> user.
>>
>> A side-effect is that some offload settings (typically UFO)
>> may be enabled on the vlan device while being disabled on the HW.
>> However, the GSO code will correctly process the packets. This
>> actually results in slightly better raw throughput.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  net/8021q/vlan_dev.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
>> index 9ee5787..ffc8167 100644
>> --- a/net/8021q/vlan_dev.c
>> +++ b/net/8021q/vlan_dev.c
>> @@ -626,10 +626,16 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>>  {
>>         struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
>>         netdev_features_t old_features = features;
>> +       netdev_features_t real_dev_features = real_dev->features;
>>
>> -       features = netdev_intersect_features(features, real_dev->vlan_features);
>> +       features = netdev_intersect_features(features,
>> +                                            (real_dev->vlan_features |
>> +                                             NETIF_F_HW_CSUM));
> 
> You might want to change the ordering on all this.
> 
> You could start out with a value based on the intersection of
> real_dev->features and real_dev->vlan_features. Then you don't need to
> mess around with this extra piece where you are having OR in HW_CSUM.

You know,  I did that and that was the patch I meant to send... I had
3 different versions of this thing trying to find the best way...

Let me repost, since some of the rest of the changes go away.

-vlad

> That way you don't risk losing track of the state of the hardware
> checksum offload in terms of vlan_features as it should completely
> clear all of the checksums if none of them are supported in hardware.
> 
>>         features |= NETIF_F_RXCSUM;
> 
> This line would probably need to be changed to OR NETIF_F_RXCSUM with
> the real_dev->vlan_features when we perform the first intersect test.
> That way we are guaranteed to report RXCSUM if the underlying device
> supports it. It might just be worth while to force this into the
> vlan_features for all devices in register_netdevice() then we wouldn't
> need this line at all and it probably makes sense since it would allow
> us to save a few extra cycles/instructions by combining it with the
> line that was adding high dma support.
> 
>> -       features = netdev_intersect_features(features, real_dev->features);
>> +       if (real_dev_features & (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM))
>> +               real_dev_features |= NETIF_F_HW_CSUM;
>> +
>> +       features = netdev_intersect_features(features, real_dev_features);
> 
> This part all looks good.
> 
> My only advice like I said would be to record the vlan_features
> intersection first based on the real_dev. That way you don't risk
> losing state data from real device if for some reason it doesn't
> support checksum offload with VLAN tagged frames.
> 
>>         features |= old_features & (NETIF_F_SOFT_FEATURES | NETIF_F_GSO_SOFTWARE);
>>         features |= NETIF_F_LLTX;
>> --
>> 2.7.4
>>