netdev - Re: [PATCH v3] net: Allow no-cache copy from user on transmit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTinHh6L7mtKp3G5x-6e7Z5-pTdvjYw@mail.gmail.com>
Date:	Thu, 31 Mar 2011 21:05:30 -0700
From:	Tom Herbert <therbert@...gle.com>
To:	Michał Mirosław <mirqus@...il.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v3] net: Allow no-cache copy from user on transmit

> BTW, the checks should take protocol into account. If, for example,
> device has NOCACHE_COPY and CSUM_IPV4, but not CSUM_IPV6, then it will
> take a hit for every IPv6 packet that needs checksumming in software.

If the HW does not do checksum for a protocol, then the checksum-copy
path is taken for packets so there should be no hit.  The logic of the
default setting is that if HW does not checksum offload, then there is
likely no benefit to turning on no cache copy; all the packets would
go through copy checksum.

> Maybe this can be taken into account when sk_route_caps are set?
>
> [...]
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 1a6e9eb..59b6ce9 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1407,7 +1407,7 @@ static int bond_compute_features(struct bonding *bond)
>>        int i;
>>
>>        features &= ~(NETIF_F_ALL_CSUM | BOND_VLAN_FEATURES);
>> -       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM;
>> +       features |=  NETIF_F_GSO_MASK | NETIF_F_NO_CSUM | NETIF_F_NOCACHE_COPY;
>>
>>        if (!bond->first_slave)
>>                goto done;
> [...]
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 0b88eba..454abfd 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5444,6 +5444,13 @@ int register_netdevice(struct net_device *dev)
>>                dev->features &= ~NETIF_F_GSO;
>>        }
>>
>> +       /* Turn no cache copy off if HW isn't doing checksum */
>> +       if (!(dev->features & NETIF_F_ALL_CSUM) ||
>> +           (dev->features & NETIF_F_NO_CSUM)) {
>> +               dev->wanted_features &= ~NETIF_F_NOCACHE_COPY;
>> +               dev->features &= ~NETIF_F_NOCACHE_COPY;
>> +       }
>> +
>>        /* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
>>         * vlan_dev_init() will do the dev->features check, so these features
>>         * are enabled only if supported by underlying device.
>
> Why NO_CSUM is disabling NOCACHE_COPY? You combine those features in
> bonding code anyway.
>
This is likely loopback in which case no cache copy probably does not
help.  For bonding, no cache copy is only enabled when all slaves are
doing it.

> You didn't add netdev_fix_features() checks, so NOCACHE_COPY might be
> enabled via ethtool after device is registered even if driver doesn't
> support checksum offloads.
>
I don't see a reason to strictly enforce this.  It's not guaranteed
that if a driver doesn't support csum offload then no cache copy is
bad (again it's probably noop anyway because of copy-csum path).
Turning it on for loopback is a benefit if sender and receiver are in
different cache domains is good.

I do need to send another patch.  The skb_add_data function can also
use this.  I will create skb_do_copy_data_nocache and
skb_add_data_nocache to make the use of this explicit in each protocol
(TCP being the first supported).

Tom
--
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