* [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
@ 2017-04-20 1:12 Vladislav Yasevich
2017-04-20 6:13 ` Michal Kubecek
2017-04-20 18:36 ` Alexander Duyck
0 siblings, 2 replies; 5+ messages in thread
From: Vladislav Yasevich @ 2017-04-20 1:12 UTC (permalink / raw)
To: netdev; +Cc: tom, mkubecek, Vladislav Yasevich
While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
This results in an interesting situation when the software
device is configured on top of hw device using (IP|IPV6)_CSUM.
In this situation, the user can't turn off checksum offloading
features on the software device.
This patch resolves that by prefering the NETIF_F_HW_CSUM setting
when computing a feature intersect.
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
include/linux/netdevice.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 97456b25..3d811c1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
{
if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
if (f1 & NETIF_F_HW_CSUM)
- f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+ f2 |= NETIF_F_HW_CSUM;
else
- f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
+ f1 |= NETIF_F_HW_CSUM;
}
return f1 & f2;
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
2017-04-20 1:12 [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features Vladislav Yasevich
@ 2017-04-20 6:13 ` Michal Kubecek
2017-04-20 15:20 ` Vlad Yasevich
2017-04-20 18:36 ` Alexander Duyck
1 sibling, 1 reply; 5+ messages in thread
From: Michal Kubecek @ 2017-04-20 6:13 UTC (permalink / raw)
To: Vladislav Yasevich; +Cc: netdev, tom, Vladislav Yasevich
On Wed, Apr 19, 2017 at 09:12:23PM -0400, Vladislav Yasevich wrote:
> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
> This results in an interesting situation when the software
> device is configured on top of hw device using (IP|IPV6)_CSUM.
> In this situation, the user can't turn off checksum offloading
> features on the software device.
>
> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
> when computing a feature intersect.
The reasoning makes sense to me but perhaps we should rename the helper
then to make it obvious that it doesn't work like intersection anymore
(not even in the logical sense).
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 97456b25..3d811c1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
> {
> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
> if (f1 & NETIF_F_HW_CSUM)
> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f2 |= NETIF_F_HW_CSUM;
> else
> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f1 |= NETIF_F_HW_CSUM;
> }
>
> return f1 & f2;
I hope I didn't miss something but it seems we can avoid nested ifs now:
if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
f1 |= NETIF_F_HW_CSUM;
f2 |= NETIF_F_HW_CSUM;
}
Michal Kubecek
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
2017-04-20 6:13 ` Michal Kubecek
@ 2017-04-20 15:20 ` Vlad Yasevich
0 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2017-04-20 15:20 UTC (permalink / raw)
To: Michal Kubecek, Vladislav Yasevich; +Cc: netdev, tom
On 04/20/2017 02:13 AM, Michal Kubecek wrote:
> On Wed, Apr 19, 2017 at 09:12:23PM -0400, Vladislav Yasevich wrote:
>> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
>> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
>> This results in an interesting situation when the software
>> device is configured on top of hw device using (IP|IPV6)_CSUM.
>> In this situation, the user can't turn off checksum offloading
>> features on the software device.
>>
>> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
>> when computing a feature intersect.
>
> The reasoning makes sense to me but perhaps we should rename the helper
> then to make it obvious that it doesn't work like intersection anymore
> (not even in the logical sense).
It's still an intersect in the end since we return (f1 & f2). We are just
munging checksum so that we get a HW_CSUM in the end if one of the devices supported
it.
>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 97456b25..3d811c1 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
>> {
>> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
>> if (f1 & NETIF_F_HW_CSUM)
>> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f2 |= NETIF_F_HW_CSUM;
>> else
>> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f1 |= NETIF_F_HW_CSUM;
>> }
>>
>> return f1 & f2;
>
> I hope I didn't miss something but it seems we can avoid nested ifs now:
>
> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
> f1 |= NETIF_F_HW_CSUM;
> f2 |= NETIF_F_HW_CSUM;
> }
>
Yes, we can do that. We could have done something like this before as well, I suppose.
Thanks
-vlad
> Michal Kubecek
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
2017-04-20 1:12 [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features Vladislav Yasevich
2017-04-20 6:13 ` Michal Kubecek
@ 2017-04-20 18:36 ` Alexander Duyck
2017-04-20 19:07 ` Vlad Yasevich
1 sibling, 1 reply; 5+ messages in thread
From: Alexander Duyck @ 2017-04-20 18:36 UTC (permalink / raw)
To: Vladislav Yasevich
Cc: Netdev, Tom Herbert, Michal Kubecek, Vladislav Yasevich
On Wed, Apr 19, 2017 at 6:12 PM, Vladislav Yasevich <vyasevich@gmail.com> wrote:
> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
> This results in an interesting situation when the software
> device is configured on top of hw device using (IP|IPV6)_CSUM.
> In this situation, the user can't turn off checksum offloading
> features on the software device.
>
> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
> when computing a feature intersect.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
> include/linux/netdevice.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 97456b25..3d811c1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
> {
> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
> if (f1 & NETIF_F_HW_CSUM)
> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f2 |= NETIF_F_HW_CSUM;
> else
> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> + f1 |= NETIF_F_HW_CSUM;
> }
>
> return f1 & f2;
> --
> 2.7.4
>
This doesn't seem right to me. I can see promoting NETIF_F_HW_CSUM to
also include IP_CSUM and IPV6_CSUM but all this seems to do is force
NETIF_F_HW_CSUM on always if either side has it enabled. That doesn't
seem like an intersection at all to me.
What this code is supposed to do is make it so that if we are testing
for NETIF_F_IP_CSUM and NETIF_F_HW_CSUM is set it will return true.
The code as it is now will always return true if we test for
NETIF_F_HW_CSUM since it will just set it on the other side for us.
- Alex
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features
2017-04-20 18:36 ` Alexander Duyck
@ 2017-04-20 19:07 ` Vlad Yasevich
0 siblings, 0 replies; 5+ messages in thread
From: Vlad Yasevich @ 2017-04-20 19:07 UTC (permalink / raw)
To: Alexander Duyck, Vladislav Yasevich; +Cc: Netdev, Tom Herbert, Michal Kubecek
On 04/20/2017 02:36 PM, Alexander Duyck wrote:
> On Wed, Apr 19, 2017 at 6:12 PM, Vladislav Yasevich <vyasevich@gmail.com> wrote:
>> While hardware device use either NETIF_F_(IP|IPV6)_CSUM or
>> NETIF_F_HW_CSUM, all of the software devices use HW_CSUM.
>> This results in an interesting situation when the software
>> device is configured on top of hw device using (IP|IPV6)_CSUM.
>> In this situation, the user can't turn off checksum offloading
>> features on the software device.
>>
>> This patch resolves that by prefering the NETIF_F_HW_CSUM setting
>> when computing a feature intersect.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>> include/linux/netdevice.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 97456b25..3d811c1 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -4019,9 +4019,9 @@ static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
>> {
>> if ((f1 ^ f2) & NETIF_F_HW_CSUM) {
>> if (f1 & NETIF_F_HW_CSUM)
>> - f1 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f2 |= NETIF_F_HW_CSUM;
>> else
>> - f2 |= (NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>> + f1 |= NETIF_F_HW_CSUM;
>> }
>>
>> return f1 & f2;
>> --
>> 2.7.4
>>
>
> This doesn't seem right to me. I can see promoting NETIF_F_HW_CSUM to
> also include IP_CSUM and IPV6_CSUM but all this seems to do is force
> NETIF_F_HW_CSUM on always if either side has it enabled. That doesn't
> seem like an intersection at all to me.
I think it needs to be the other way around. At least that's what I've
been observing...
>
> What this code is supposed to do is make it so that if we are testing
> for NETIF_F_IP_CSUM and NETIF_F_HW_CSUM is set it will return true.
> The code as it is now will always return true if we test for
> NETIF_F_HW_CSUM since it will just set it on the other side for us.
>
Yes, you are right. The way the code is now, if we turn off checksum
offload on the lower device, the upper device doesn't change.
What we really want to do is add HW_CSUM to the mask if IP|IPV6 is set.
That way, if lower device has IP|IPV6, it will end up with IP|IPV6|HW.
This way, controlling IP csums on the lower device will turn off the
checksum on the upper, while at the same time give us the ability to
control the checksum offload on the upper device itself.
Thanks. I'll resubmit a v2.
-vlad
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-20 19:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-20 1:12 [PATCH net] netdevice: Prefer NETIF_F_HW_CSUM when intersecting features Vladislav Yasevich
2017-04-20 6:13 ` Michal Kubecek
2017-04-20 15:20 ` Vlad Yasevich
2017-04-20 18:36 ` Alexander Duyck
2017-04-20 19:07 ` Vlad Yasevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).