* [PATCH] gre: fix return value of gre_rcv
From: Haishuang Yan @ 2016-03-23 1:34 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, James Morris, Patrick McHardy
Cc: netdev, linux-kernel, Haishuang Yan
Dropped skb's should be documented by an appropriate return value.
Use the correct NET_RX_DROP and NET_RX_SUCCESS values for that reason.
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
net/ipv4/ip_gre.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 31936d3..1dc0cdb 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -432,12 +432,12 @@ static int gre_rcv(struct sk_buff *skb)
goto drop;
if (ipgre_rcv(skb, &tpi) == PACKET_RCVD)
- return 0;
+ return NET_RX_SUCCESS;
icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);
drop:
kfree_skb(skb);
- return 0;
+ return NET_RX_DROP;
}
static __sum16 gre_checksum(struct sk_buff *skb)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net v2] xfrm: Fix crash observed during device unregistration and decryption
From: Subash Abhinov Kasiviswanathan @ 2016-03-23 0:29 UTC (permalink / raw)
To: 'Steffen Klassert'; +Cc: netdev, 'Herbert Xu', jeromes
A crash is observed when a decrypted packet is processed in receive
path. get_rps_cpus() tries to dereference the skb->dev fields but it
appears that the device is freed from the poison pattern.
[<ffffffc000af58ec>] get_rps_cpu+0x94/0x2f0
[<ffffffc000af5f94>] netif_rx_internal+0x140/0x1cc
[<ffffffc000af6094>] netif_rx+0x74/0x94
[<ffffffc000bc0b6c>] xfrm_input+0x754/0x7d0
[<ffffffc000bc0bf8>] xfrm_input_resume+0x10/0x1c
[<ffffffc000ba6eb8>] esp_input_done+0x20/0x30
[<ffffffc0000b64c8>] process_one_work+0x244/0x3fc
[<ffffffc0000b7324>] worker_thread+0x2f8/0x418
[<ffffffc0000bb40c>] kthread+0xe0/0xec
-013|get_rps_cpu(
| dev = 0xFFFFFFC08B688000,
| skb = 0xFFFFFFC0C76AAC00 -> (
| dev = 0xFFFFFFC08B688000 -> (
| name =
"......................................................
| name_hlist = (next = 0xAAAAAAAAAAAAAAAA, pprev =
0xAAAAAAAAAAA
Following are the sequence of events observed -
- Encrypted packet in receive path from netdevice is queued
- Encrypted packet queued for decryption (asynchronous)
- Netdevice brought down and freed
- Packet is decrypted and returned through callback in esp_input_done
- Packet is queued again for process in network stack using netif_rx
Since the device appears to have been freed, the dereference of
skb->dev in get_rps_cpus() leads to an unhandled page fault
exception.
Fix this by holding on to device reference when queueing packets
asynchronously and releasing the reference on call back return.
v2: Make the change generic to xfrm as mentioned by Steffen and
update the title to xfrm
Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Jerome Stanislaus <jeromes@codeaurora.org>
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
net/xfrm/xfrm_input.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index ad7f5b3..1c4ad47 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -292,12 +292,15 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32
spi, int encap_type)
XFRM_SKB_CB(skb)->seq.input.hi = seq_hi;
skb_dst_force(skb);
+ dev_hold(skb->dev);
nexthdr = x->type->input(x, skb);
if (nexthdr == -EINPROGRESS)
return 0;
resume:
+ dev_put(skb->dev);
+
spin_lock(&x->lock);
if (nexthdr <= 0) {
if (nexthdr == -EBADMSG) {
^ permalink raw reply related
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Eric Dumazet @ 2016-03-22 23:36 UTC (permalink / raw)
To: Cong Wang
Cc: Wei Wang, Martin KaFai Lau, Wei Wang, David Miller, Eric Dumazet,
Linux Kernel Network Developers
In-Reply-To: <CAM_iQpU7JeLuAVPqtzjvJ7qvMYTDZSR8xc7Vfw--uQDxW-WaHQ@mail.gmail.com>
On Tue, 2016-03-22 at 13:13 -0700, Cong Wang wrote:
> On Tue, Mar 22, 2016 at 11:03 AM, Wei Wang <tracywwnj@gmail.com> wrote:
> > Thanks Martin and Cong.
> >
> > I guess then we are going with the following fix in ip6_sk_update_pmtu():
> > 1. call ip6_upate_pmtu() as it is
> > 2. do a dst_check()
> > 3. re-lookup() if it is invalid
> > 4. and then do a ip6_dst_store()/dst_set
>
> Exactly, please try the attached patch. Note I did nothing more than a
> compile test.
>
> Does it make sense to you now?
Hard to reply on your patch as it was not inlined.
1) Lot of code duplication, for some reason I do not yet understand.
ip6_sk_update_pmtu() and ip6_update_pmtu() will basically do the same
thing...
2)
+ if (sk->sk_state == TCP_ESTABLISHED)
+ ip6_dst_store(sk, dst, &iph->daddr, &iph->saddr);
+out:
ip6_dst_store() will do :
np->daddr_cache = daddr; (&iph->daddr)
np->saddr_cache = saddr; (&iph->saddr)
So when skb is freed, daddr_cache & saddr_cache point to freed data.
^ permalink raw reply
* [net PATCH] net: Reset encap_level to avoid resetting features on inner IP headers
From: Alexander Duyck @ 2016-03-22 23:18 UTC (permalink / raw)
To: tom, kernel-team, davem, alexander.duyck, netdev
This patch corrects an oversight in which we were allowing the encap_level
value to pass from the outer headers to the inner headers. As a result we
were incorrectly identifying UDP or GRE tunnels as also making use of ipip
or sit when the second header actually represented a tunnel encapsulated in
either a UDP or GRE tunnel which already had the features masked.
Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
Reported-by: Tom Herbert <tom@herbertland.com>
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
net/ipv4/gre_offload.c | 1 +
net/ipv4/udp_offload.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index dea0390d65bb..43a5c76742dc 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -49,6 +49,7 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
/* setup inner skb. */
skb->encapsulation = 0;
+ SKB_GSO_CB(skb)->encap_level = 0;
__skb_pull(skb, tnl_hlen);
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb_inner_network_offset(skb));
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 5fcb93269afb..80236a9e1769 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -56,6 +56,7 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
/* setup inner skb. */
skb->encapsulation = 0;
+ SKB_GSO_CB(skb)->encap_level = 0;
__skb_pull(skb, tnl_hlen);
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb_inner_network_offset(skb));
^ permalink raw reply related
* [PATCH] net: mediatek: fix checking for NULL instead of IS_ERR() in .probe
From: Vladimir Zapolskiy @ 2016-03-22 23:06 UTC (permalink / raw)
To: David S. Miller, Felix Fietkau, John Crispin
Cc: netdev, linux-arm-kernel, linux-mediatek
devm_ioremap_resource() returns ERR_PTR() value on error, it never
returns NULL, fix it and propagate the returned error upwards.
Fixes: 656e705243fd ("net-next: mediatek: add support for MT7623 ethernet")
Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
drivers/net/ethernet/mediatek/mtk_eth_soc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 7f2126b..e0b68af 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -1690,8 +1690,8 @@ static int mtk_probe(struct platform_device *pdev)
return -ENOMEM;
eth->base = devm_ioremap_resource(&pdev->dev, res);
- if (!eth->base)
- return -EADDRNOTAVAIL;
+ if (IS_ERR(eth->base))
+ return PTR_ERR(eth->base);
spin_lock_init(ð->page_lock);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net-next] net: Fix remote checksum offload with GUE
From: Tom Herbert @ 2016-03-22 22:16 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Kernel Team, Alex Duyck
In-Reply-To: <CALx6S35waJuT4WrVnCVh03E8XfPQh64jXWTwWa2Ezd-wNwsRfA@mail.gmail.com>
On Tue, Mar 22, 2016 at 3:09 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Mar 22, 2016 at 2:45 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>>> In skb_segment the check of whether or not to perform the checksum on
>>>>> host was changed to not consider rather remote checksum offload is
>>>>> in use. In the case that can_checksum_protocol fails the checksum
>>>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>>>> the enc_features and remote checksum offload is being done. The
>>>>> problem is that this bit can be cleared in lower GSO layers that
>>>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>>>> so when we get to skb_segment that intent has been lost and
>>>>> can_checksum_protocol fails.
>>>>
>>>> So what you are describing sounds like a tunnel in tunnel scenario.
>>>> It might work better to just skip masking the features if
>>>> skb->remcsum_offload is set rather than trying to change how we
>>>> perform the offload.
>>>>
>>> To be clear, my patch is restoring the old behavior not implementing a new one.
>>
>> Yes, but the old behavior could lead to kernel panics under certain
>> circumstances.
>>
>>>> I'm pretty sure this will cause data corruption and maybe a kernel
>>>> panic if Tx checksum offload is disabled.
>>>>
>>> Nope, working fine for me.
>>
>> What are the options you used to create the tunnel? Did you enable
>> both remcsum and udpcsum?
>>
> modprobe fou
> ./ip fou add port 6080 gue
> ./ip link add name tun1 type ipip remote 10.1.1.2 local 10.1.1.2 ttl
> 225 encap gue encap-sport auto encap-dport 6080 encap-csum
> encap-remcsum
> ifconfig tun1 192.168.1.1
> ip route add 192.168.1.0/24 dev tun1
>
Here are the command I use for VXLAN for reference:
./ip link add vxlan0 type vxlan id 10 group 224.10.10.10 ttl 10 dev
eth0 udpcsum remcsumtx remcsumrx
ifconfig vxlan0 192.168.111.1
ip route add 192.168.111.0/24 dev vxlan0
When remcsum offload is working properly (enabled and outer checksum
is offloaded) then csum_partial gets very few cycles as shown by perf.
Tom
> Thanks,
> Tom
>
>>> The outer checksum would be computed in
>>>>> This patch:
>>>>>
>>>>> - Restores the check in skb_segment to look at remote checksum offload.
>>>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>>>> NETIF_F_HW_CSUM feature since this is no longer useful with above
>>>>> change.
>>>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>>>> A special case should not be needed here.
>>>>>
>>>>> Tested: Single netperf STREAM over GUE-ipip
>>>>>
>>>>> Before fix:
>>>>> 5625 Mbps
>>>>> After fix:
>>>>> 6410 Mbps
>>>>>
>>>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>>>> ---
>>>>> include/linux/skbuff.h | 4 ----
>>>>> net/core/skbuff.c | 5 ++---
>>>>> net/ipv4/udp_offload.c | 10 ----------
>>>>> 3 files changed, 2 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>>> index 15d0df9..f6fe8a5 100644
>>>>> --- a/include/linux/skbuff.h
>>>>> +++ b/include/linux/skbuff.h
>>>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>>>
>>>>> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>>>> {
>>>>> - /* Do not update partial checksums if remote checksum is enabled. */
>>>>> - if (skb->remcsum_offload)
>>>>> - return;
>>>>> -
>>>>> SKB_GSO_CB(skb)->csum = res;
>>>>> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>>>> }
>>>>
>>>> I'm pretty sure this part here will break things when you don't have
>>>> an outer offload enabled. What NIC did you test this on? Did you try
>>>> disabling the Tx checksum support in the hardware to see what would
>>>> happen?
>>>>
>>> Mellanox.
>>>
>>> When TX checksum is disabled the outer checksum is computed in
>>> __skb_udp_tunnel_segment.
>>
>> Yes, but that uses gso_make_checksum which requires the csum and
>> csum_start fields be initialized in the SKB_GSO_CB().
>>
>>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>>> index f044f97..e4eb78d 100644
>>>>> --- a/net/core/skbuff.c
>>>>> +++ b/net/core/skbuff.c
>>>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>>>> nskb->truesize += nskb->data_len;
>>>>>
>>>>> perform_csum_check:
>>>>> - if (!csum) {
>>>>> + if (!csum && !nskb->remcsum_offload) {
>>>>> if (skb_has_shared_frag(nskb)) {
>>>>> err = __skb_linearize(nskb);
>>>>> if (err)
>>>>> goto err;
>>>>> }
>>>>> - if (!nskb->remcsum_offload)
>>>>> - nskb->ip_summed = CHECKSUM_NONE;
>>>>> + nskb->ip_summed = CHECKSUM_NONE;
>>>>> SKB_GSO_CB(nskb)->csum =
>>>>> skb_checksum(nskb, doffset,
>>>>> nskb->len - doffset, 0);
>>>>
>>>> I'm pretty sure this is going to cause a huge mess if you are
>>>> requesting remote checksum but cannot perform an outer checksum. One
>>>> of the reasons I merged these features together the way I did was
>>>> because we needed to perform the initial checksum to avoid causing a
>>>> kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
>>>> and SKB_GSO_CB()->csum_start fields populated.
>>>>
>>> Nope, if we can't perform outer checksum it is done in the host. I'm
>>> not seeing any problem.
>>
>> Are you running remcsum with udpcsum set or is udpcsum not set? If
>> you don't set udpcsum it doesn't actually compute an outer checksum
>> value.
>>
>>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>>> index 8a3405a..f86f1e1 100644
>>>>> --- a/net/ipv4/udp_offload.c
>>>>> +++ b/net/ipv4/udp_offload.c
>>>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>>>
>>>>> features &= skb->dev->hw_enc_features;
>>>>>
>>>>> - /* The only checksum offload we care about from here on out is the
>>>>> - * outer one so strip the existing checksum feature flags and
>>>>> - * instead set the flag based on our outer checksum offload value.
>>>>> - */
>>>>> - if (remcsum || ufo) {
>>>>> - features &= ~NETIF_F_CSUM_MASK;
>>>>> - if (!need_csum || offload_csum)
>>>>> - features |= NETIF_F_HW_CSUM;
>>>>> - }
>>>>> -
>>>>> /* segment inner packet. */
>>>>> segs = gso_inner_segment(skb, features);
>>>>> if (IS_ERR_OR_NULL(segs)) {
>>>>
>>>> This part breaks UDP fragmentation I am pretty sure. We need this bit
>>>> for UFO to avoid having to perform a checksum over the data twice if
>>>> we are offloading the outer UDP checksum.
>>>
>>> The maybe leave this for ufo if it is a problem (please test your
>>> suppositions!), but this is not good for remcsum offload.
>>
>> I suspect we are supposed to be setting encap_level to 0 when we set
>> skb->encapsulation to 0 before processing inner headers in the case of
>> UDP and GRE tunnel offloads. I'll try to test and submit something in
>> the next hour or two.
>>
>> - Alex
^ permalink raw reply
* [PATCH v2] prism54: isl_38xx: Replace 'struct timeval'
From: Tina Ruchandani @ 2016-03-22 22:16 UTC (permalink / raw)
To: y2038; +Cc: linux-wireless, netdev, arnd, linux-kernel, Kalle Valo
'struct timeval' uses a 32-bit seconds field which will overflow in
year 2038 and beyond. This patch is part of a larger effort to remove
all instances of 'struct timeval' from the kernel and replace them
with 64-bit timekeeping variables.
The correctness of the code isn't affected by this patch - the seconds
value being printed would earlier be wrong due to overflow in timeval,
and now it gets truncated to 32-bit due to the 'long' cast used on
tv.sec field to prevent compiler warnings. Truly fixing this would
require changing the debug print to print more than 8 digits and
use a different specifier from %li.
The patch was build-tested / debugged by removing the
"if VERBOSE > SHOW_ERROR_MESSAGES" guards.
Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
--
Changes in v2:
- Changed printf specifier as suggested by Arnd Bergmann to
avoid truncation.
---
drivers/net/wireless/intersil/prism54/isl_38xx.c | 35 ++++++++++++------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/net/wireless/intersil/prism54/isl_38xx.c b/drivers/net/wireless/intersil/prism54/isl_38xx.c
index 333c1a2..6700387 100644
--- a/drivers/net/wireless/intersil/prism54/isl_38xx.c
+++ b/drivers/net/wireless/intersil/prism54/isl_38xx.c
@@ -19,6 +19,7 @@
#include <linux/module.h>
#include <linux/types.h>
#include <linux/delay.h>
+#include <linux/ktime.h>
#include <asm/uaccess.h>
#include <asm/io.h>
@@ -113,7 +114,7 @@ isl38xx_trigger_device(int asleep, void __iomem *device_base)
#if VERBOSE > SHOW_ERROR_MESSAGES
u32 counter = 0;
- struct timeval current_time;
+ struct timespec64 current_ts64;
DEBUG(SHOW_FUNCTION_CALLS, "isl38xx trigger device\n");
#endif
@@ -121,22 +122,22 @@ isl38xx_trigger_device(int asleep, void __iomem *device_base)
if (asleep) {
/* device is in powersave, trigger the device for wakeup */
#if VERBOSE > SHOW_ERROR_MESSAGES
- do_gettimeofday(¤t_time);
- DEBUG(SHOW_TRACING, "%08li.%08li Device wakeup triggered\n",
- current_time.tv_sec, (long)current_time.tv_usec);
+ ktime_get_real_ts64(¤t_ts64);
+ DEBUG(SHOW_TRACING, "%lld.%09ld Device wakeup triggered\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec);
- DEBUG(SHOW_TRACING, "%08li.%08li Device register read %08x\n",
- current_time.tv_sec, (long)current_time.tv_usec,
+ DEBUG(SHOW_TRACING, "%lld.%09ld Device register read %08x\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec,
readl(device_base + ISL38XX_CTRL_STAT_REG));
#endif
reg = readl(device_base + ISL38XX_INT_IDENT_REG);
if (reg == 0xabadface) {
#if VERBOSE > SHOW_ERROR_MESSAGES
- do_gettimeofday(¤t_time);
+ ktime_get_real_ts64(¤t_ts64);
DEBUG(SHOW_TRACING,
- "%08li.%08li Device register abadface\n",
- current_time.tv_sec, (long)current_time.tv_usec);
+ "%lld.%09ld Device register abadface\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec);
#endif
/* read the Device Status Register until Sleepmode bit is set */
while (reg = readl(device_base + ISL38XX_CTRL_STAT_REG),
@@ -149,13 +150,13 @@ isl38xx_trigger_device(int asleep, void __iomem *device_base)
#if VERBOSE > SHOW_ERROR_MESSAGES
DEBUG(SHOW_TRACING,
- "%08li.%08li Device register read %08x\n",
- current_time.tv_sec, (long)current_time.tv_usec,
+ "%lld.%09ld Device register read %08x\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec,
readl(device_base + ISL38XX_CTRL_STAT_REG));
- do_gettimeofday(¤t_time);
+ ktime_get_real_ts64(¤t_ts64);
DEBUG(SHOW_TRACING,
- "%08li.%08li Device asleep counter %i\n",
- current_time.tv_sec, (long)current_time.tv_usec,
+ "%lld.%09ld Device asleep counter %i\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec,
counter);
#endif
}
@@ -168,9 +169,9 @@ isl38xx_trigger_device(int asleep, void __iomem *device_base)
/* perform another read on the Device Status Register */
reg = readl(device_base + ISL38XX_CTRL_STAT_REG);
- do_gettimeofday(¤t_time);
- DEBUG(SHOW_TRACING, "%08li.%08li Device register read %08x\n",
- current_time.tv_sec, (long)current_time.tv_usec, reg);
+ ktime_get_real_ts64(¤t_ts64);
+ DEBUG(SHOW_TRACING, "%lld.%00ld Device register read %08x\n",
+ (s64)current_ts64.tv_sec, current_ts64.tv_nsec, reg);
#endif
} else {
/* device is (still) awake */
^ permalink raw reply related
* Re: [PATCH net-next] net: Fix remote checksum offload with GUE
From: Tom Herbert @ 2016-03-22 22:09 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Kernel Team, Alex Duyck
In-Reply-To: <CAKgT0UeAiYHX7n4Y8S0QqXSExuzpQCQJvBCNWOtF-uLn8Qnc0g@mail.gmail.com>
On Tue, Mar 22, 2016 at 2:45 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
>> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>>> In skb_segment the check of whether or not to perform the checksum on
>>>> host was changed to not consider rather remote checksum offload is
>>>> in use. In the case that can_checksum_protocol fails the checksum
>>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>>> the enc_features and remote checksum offload is being done. The
>>>> problem is that this bit can be cleared in lower GSO layers that
>>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>>> so when we get to skb_segment that intent has been lost and
>>>> can_checksum_protocol fails.
>>>
>>> So what you are describing sounds like a tunnel in tunnel scenario.
>>> It might work better to just skip masking the features if
>>> skb->remcsum_offload is set rather than trying to change how we
>>> perform the offload.
>>>
>> To be clear, my patch is restoring the old behavior not implementing a new one.
>
> Yes, but the old behavior could lead to kernel panics under certain
> circumstances.
>
>>> I'm pretty sure this will cause data corruption and maybe a kernel
>>> panic if Tx checksum offload is disabled.
>>>
>> Nope, working fine for me.
>
> What are the options you used to create the tunnel? Did you enable
> both remcsum and udpcsum?
>
modprobe fou
./ip fou add port 6080 gue
./ip link add name tun1 type ipip remote 10.1.1.2 local 10.1.1.2 ttl
225 encap gue encap-sport auto encap-dport 6080 encap-csum
encap-remcsum
ifconfig tun1 192.168.1.1
ip route add 192.168.1.0/24 dev tun1
Thanks,
Tom
>> The outer checksum would be computed in
>>>> This patch:
>>>>
>>>> - Restores the check in skb_segment to look at remote checksum offload.
>>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>>> NETIF_F_HW_CSUM feature since this is no longer useful with above
>>>> change.
>>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>>> A special case should not be needed here.
>>>>
>>>> Tested: Single netperf STREAM over GUE-ipip
>>>>
>>>> Before fix:
>>>> 5625 Mbps
>>>> After fix:
>>>> 6410 Mbps
>>>>
>>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>>> ---
>>>> include/linux/skbuff.h | 4 ----
>>>> net/core/skbuff.c | 5 ++---
>>>> net/ipv4/udp_offload.c | 10 ----------
>>>> 3 files changed, 2 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index 15d0df9..f6fe8a5 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>>
>>>> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>>> {
>>>> - /* Do not update partial checksums if remote checksum is enabled. */
>>>> - if (skb->remcsum_offload)
>>>> - return;
>>>> -
>>>> SKB_GSO_CB(skb)->csum = res;
>>>> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>>> }
>>>
>>> I'm pretty sure this part here will break things when you don't have
>>> an outer offload enabled. What NIC did you test this on? Did you try
>>> disabling the Tx checksum support in the hardware to see what would
>>> happen?
>>>
>> Mellanox.
>>
>> When TX checksum is disabled the outer checksum is computed in
>> __skb_udp_tunnel_segment.
>
> Yes, but that uses gso_make_checksum which requires the csum and
> csum_start fields be initialized in the SKB_GSO_CB().
>
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index f044f97..e4eb78d 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>>> nskb->truesize += nskb->data_len;
>>>>
>>>> perform_csum_check:
>>>> - if (!csum) {
>>>> + if (!csum && !nskb->remcsum_offload) {
>>>> if (skb_has_shared_frag(nskb)) {
>>>> err = __skb_linearize(nskb);
>>>> if (err)
>>>> goto err;
>>>> }
>>>> - if (!nskb->remcsum_offload)
>>>> - nskb->ip_summed = CHECKSUM_NONE;
>>>> + nskb->ip_summed = CHECKSUM_NONE;
>>>> SKB_GSO_CB(nskb)->csum =
>>>> skb_checksum(nskb, doffset,
>>>> nskb->len - doffset, 0);
>>>
>>> I'm pretty sure this is going to cause a huge mess if you are
>>> requesting remote checksum but cannot perform an outer checksum. One
>>> of the reasons I merged these features together the way I did was
>>> because we needed to perform the initial checksum to avoid causing a
>>> kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
>>> and SKB_GSO_CB()->csum_start fields populated.
>>>
>> Nope, if we can't perform outer checksum it is done in the host. I'm
>> not seeing any problem.
>
> Are you running remcsum with udpcsum set or is udpcsum not set? If
> you don't set udpcsum it doesn't actually compute an outer checksum
> value.
>
>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>> index 8a3405a..f86f1e1 100644
>>>> --- a/net/ipv4/udp_offload.c
>>>> +++ b/net/ipv4/udp_offload.c
>>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>>
>>>> features &= skb->dev->hw_enc_features;
>>>>
>>>> - /* The only checksum offload we care about from here on out is the
>>>> - * outer one so strip the existing checksum feature flags and
>>>> - * instead set the flag based on our outer checksum offload value.
>>>> - */
>>>> - if (remcsum || ufo) {
>>>> - features &= ~NETIF_F_CSUM_MASK;
>>>> - if (!need_csum || offload_csum)
>>>> - features |= NETIF_F_HW_CSUM;
>>>> - }
>>>> -
>>>> /* segment inner packet. */
>>>> segs = gso_inner_segment(skb, features);
>>>> if (IS_ERR_OR_NULL(segs)) {
>>>
>>> This part breaks UDP fragmentation I am pretty sure. We need this bit
>>> for UFO to avoid having to perform a checksum over the data twice if
>>> we are offloading the outer UDP checksum.
>>
>> The maybe leave this for ufo if it is a problem (please test your
>> suppositions!), but this is not good for remcsum offload.
>
> I suspect we are supposed to be setting encap_level to 0 when we set
> skb->encapsulation to 0 before processing inner headers in the case of
> UDP and GRE tunnel offloads. I'll try to test and submit something in
> the next hour or two.
>
> - Alex
^ permalink raw reply
* Re: [PATCH net-next] net: Fix remote checksum offload with GUE
From: Alexander Duyck @ 2016-03-22 21:45 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Netdev, Kernel Team, Alex Duyck
In-Reply-To: <CALx6S37yTDJL7aY66uOHH0jz1oyvJNS8fRMe-H-D9dKpiPVURw@mail.gmail.com>
On Tue, Mar 22, 2016 at 2:10 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>>> In skb_segment the check of whether or not to perform the checksum on
>>> host was changed to not consider rather remote checksum offload is
>>> in use. In the case that can_checksum_protocol fails the checksum
>>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>>> the enc_features and remote checksum offload is being done. The
>>> problem is that this bit can be cleared in lower GSO layers that
>>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>>> so when we get to skb_segment that intent has been lost and
>>> can_checksum_protocol fails.
>>
>> So what you are describing sounds like a tunnel in tunnel scenario.
>> It might work better to just skip masking the features if
>> skb->remcsum_offload is set rather than trying to change how we
>> perform the offload.
>>
> To be clear, my patch is restoring the old behavior not implementing a new one.
Yes, but the old behavior could lead to kernel panics under certain
circumstances.
>> I'm pretty sure this will cause data corruption and maybe a kernel
>> panic if Tx checksum offload is disabled.
>>
> Nope, working fine for me.
What are the options you used to create the tunnel? Did you enable
both remcsum and udpcsum?
> The outer checksum would be computed in
>>> This patch:
>>>
>>> - Restores the check in skb_segment to look at remote checksum offload.
>>> - Removes the code in __skb_udp_tunnel_segment to force the
>>> NETIF_F_HW_CSUM feature since this is no longer useful with above
>>> change.
>>> - Removes check for remote checksum offload in gso_reset_checksum.
>>> A special case should not be needed here.
>>>
>>> Tested: Single netperf STREAM over GUE-ipip
>>>
>>> Before fix:
>>> 5625 Mbps
>>> After fix:
>>> 6410 Mbps
>>>
>>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>>> ---
>>> include/linux/skbuff.h | 4 ----
>>> net/core/skbuff.c | 5 ++---
>>> net/ipv4/udp_offload.c | 10 ----------
>>> 3 files changed, 2 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index 15d0df9..f6fe8a5 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>>
>>> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>>> {
>>> - /* Do not update partial checksums if remote checksum is enabled. */
>>> - if (skb->remcsum_offload)
>>> - return;
>>> -
>>> SKB_GSO_CB(skb)->csum = res;
>>> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>>> }
>>
>> I'm pretty sure this part here will break things when you don't have
>> an outer offload enabled. What NIC did you test this on? Did you try
>> disabling the Tx checksum support in the hardware to see what would
>> happen?
>>
> Mellanox.
>
> When TX checksum is disabled the outer checksum is computed in
> __skb_udp_tunnel_segment.
Yes, but that uses gso_make_checksum which requires the csum and
csum_start fields be initialized in the SKB_GSO_CB().
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index f044f97..e4eb78d 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>>> nskb->truesize += nskb->data_len;
>>>
>>> perform_csum_check:
>>> - if (!csum) {
>>> + if (!csum && !nskb->remcsum_offload) {
>>> if (skb_has_shared_frag(nskb)) {
>>> err = __skb_linearize(nskb);
>>> if (err)
>>> goto err;
>>> }
>>> - if (!nskb->remcsum_offload)
>>> - nskb->ip_summed = CHECKSUM_NONE;
>>> + nskb->ip_summed = CHECKSUM_NONE;
>>> SKB_GSO_CB(nskb)->csum =
>>> skb_checksum(nskb, doffset,
>>> nskb->len - doffset, 0);
>>
>> I'm pretty sure this is going to cause a huge mess if you are
>> requesting remote checksum but cannot perform an outer checksum. One
>> of the reasons I merged these features together the way I did was
>> because we needed to perform the initial checksum to avoid causing a
>> kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
>> and SKB_GSO_CB()->csum_start fields populated.
>>
> Nope, if we can't perform outer checksum it is done in the host. I'm
> not seeing any problem.
Are you running remcsum with udpcsum set or is udpcsum not set? If
you don't set udpcsum it doesn't actually compute an outer checksum
value.
>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index 8a3405a..f86f1e1 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>>
>>> features &= skb->dev->hw_enc_features;
>>>
>>> - /* The only checksum offload we care about from here on out is the
>>> - * outer one so strip the existing checksum feature flags and
>>> - * instead set the flag based on our outer checksum offload value.
>>> - */
>>> - if (remcsum || ufo) {
>>> - features &= ~NETIF_F_CSUM_MASK;
>>> - if (!need_csum || offload_csum)
>>> - features |= NETIF_F_HW_CSUM;
>>> - }
>>> -
>>> /* segment inner packet. */
>>> segs = gso_inner_segment(skb, features);
>>> if (IS_ERR_OR_NULL(segs)) {
>>
>> This part breaks UDP fragmentation I am pretty sure. We need this bit
>> for UFO to avoid having to perform a checksum over the data twice if
>> we are offloading the outer UDP checksum.
>
> The maybe leave this for ufo if it is a problem (please test your
> suppositions!), but this is not good for remcsum offload.
I suspect we are supposed to be setting encap_level to 0 when we set
skb->encapsulation to 0 before processing inner headers in the case of
UDP and GRE tunnel offloads. I'll try to test and submit something in
the next hour or two.
- Alex
^ permalink raw reply
* [PATCH] at803x: fix reset handling
From: Sergei Shtylyov @ 2016-03-22 21:44 UTC (permalink / raw)
To: netdev, f.fainelli; +Cc: u.kleine-koenig
The driver of course "knows" that the chip's reset signal is active low,
so it drives the GPIO to 0 to reset the PHY and to 1 otherwise; however
all this will only work iff the GPIO is specified as active-high in the
device tree! I think both the driver and the device trees (if there are
any -- I was unable to find them) need to be fixed in this case...
Fixes: 13a56b449325 ("net: phy: at803x: Add support for hardware reset")
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
The patch is against DaveM's 'net.git' repo.
drivers/net/phy/at803x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Index: net/drivers/net/phy/at803x.c
===================================================================
--- net.orig/drivers/net/phy/at803x.c
+++ net/drivers/net/phy/at803x.c
@@ -277,7 +277,7 @@ static int at803x_probe(struct phy_devic
if (!priv)
return -ENOMEM;
- gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpiod_reset))
return PTR_ERR(gpiod_reset);
@@ -362,10 +362,10 @@ static void at803x_link_change_notify(st
at803x_context_save(phydev, &context);
- gpiod_set_value(priv->gpiod_reset, 0);
- msleep(1);
gpiod_set_value(priv->gpiod_reset, 1);
msleep(1);
+ gpiod_set_value(priv->gpiod_reset, 0);
+ msleep(1);
at803x_context_restore(phydev, &context);
^ permalink raw reply
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Alexander Duyck @ 2016-03-22 21:38 UTC (permalink / raw)
To: Edward Cree; +Cc: Alexander Duyck, Netdev, David Miller, Tom Herbert
In-Reply-To: <56F19FAA.3090005@solarflare.com>
On Tue, Mar 22, 2016 at 12:40 PM, Edward Cree <ecree@solarflare.com> wrote:
> On 22/03/16 17:47, Alexander Duyck wrote:
>> On Tue, Mar 22, 2016 at 10:00 AM, Edward Cree <ecree@solarflare.com> wrote:
>>> On 18/03/16 23:25, Alexander Duyck wrote:
>>>> This patch adds support for something I am referring to as GSO partial.
>>>> The basic idea is that we can support a broader range of devices for
>>>> segmentation if we use fixed outer headers and have the hardware only
>>>> really deal with segmenting the inner header. The idea behind the naming
>>>> is due to the fact that everything before csum_start will be fixed headers,
>>>> and everything after will be the region that is handled by hardware.
>>>>
>>>> With the current implementation it allows us to add support for the
>>>> following GSO types with an inner TSO or TSO6 offload:
>>>> NETIF_F_GSO_GRE
>>>> NETIF_F_GSO_GRE_CSUM
>>>> NETIF_F_UDP_TUNNEL
>>>> NETIF_F_UDP_TUNNEL_CSUM
>>>>
>>>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>>>> ---
>>> If I'm correctly understanding what you're doing, you're building a large
>>> TCP segment, feeding it through the encapsulation drivers as normal, then
>>> at GSO time you're fixing up length fields, checksums etc. in the headers.
>>> I think we can do this more simply, by making it so that at the time when
>>> we _generate_ the TCP segment, we give it headers saying it's one MSS big,
>>> but have several MSS of data. Similarly when adding the encap headers,
>>> they all need to get their lengths from what the layer below tells them,
>>> rather than the current length of data in the SKB. Then at GSO time all
>>> the headers already have the right things in, and you don't need to call
>>> any per-protocol GSO callbacks for them.
>> One issue I have to deal with here is that we have no way of knowing
>> what the underlying hardware can support at the time of segment being
>> created. You have to keep in mind that what we have access to is the
>> tunnel dev in many cases, not the underlying dev so we don't know if
>> things can be offloaded to hardware or not. By pushing this logic
>> into the GSO code we can actually implement it without much overhead
>> since we either segment it into an MSS multiple, or into single MSS
>> sized chunks. This way we defer the decision until the very last
>> moment when we actually know if we can offload some portion of this in
>> hardware or not.
> But won't the tunnel dev have the feature flag for GSO_PARTIAL depending
> on what the underlying dev advertises? (Or, at least, could we make it
> bethatway?)
This stuff doesn't work. That is why tunnels now advertise all
available features that can be offloaded via software. Basically if
we can advertise a feature we do, and then we sort things out in
software if we cannot actually do it in hardware.
> Alternatively, have per-protocol GSO callbacks to do the fixup in the
> opposite direction to what you have now - in the long term we hope that
> hardware supporting GSO partial will become the common case, so that
> should be the fast path without bouncing backwards through GSO callbacks.
> Then, if you find out at GSO time that the hardware wants to do old-style
> TSO, you call those callbacks so as to give it a superframe with the long
> lengths filled in everywhere. (Even that might not be necessary; it's a
> question of whether hardware makes assumptions about what those fields
> contain when folding its packet edits into checksums. Since this is
> going to be driver-specific and drivers doing these things will have a
> fixed list of what encaps they can parse and therefore do this for, maybe
> all these fixups could be done by the driver - using common helper
> functions, of course - in its TSO path.)
I thought about doing that but decided it was much simpler to simply
update all headers. For now I want to keep this as simple as possible
while we get the first few drivers on board. If we later want to
optimize and add complexity we can go that route, but for now this
change is more than fast enough as it allows me to push an i40e at
20Gb/s while sending frames with outer checksums enabled.
>>> Any protocol that noticed it was putting something non-copyable in its
>>> headers (e.g. GRE with the Counter field, or an outer IP layer without DF
>>> set needing real IPIDs) would set a flag in the SKB to indicate that we
>>> really do need to call through the per-protocol GSO stuff. (Ideally, if
>>> we had a separate skb->gso_start field rather than piggybacking on
>>> csum_start, we could reset it to point just before us, so that any further
>>> headers outside us still can be copied rather than taking callbacks. But
>>> I'm not sure whether that's worth using up sk_buff real estate for.)
>> The idea behind piggybacking on csum_start was due to the fact that we
>> cannot perform GSO/TSO unless CHECKSUM_PARTIAL is set. As far as I
>> know in the case of TCP offloads this always ends up being the
>> inner-most L4 header so it works out in that it actually reduces code
>> path as we were having to deal with all the skb->encapsulation checks.
>> It was a relationship that already existed, I just decided to make use
>> of it since it simplifies things pretty significantly.
> Yes; it's a clever idea. Only trouble is that we really want theinner IP
> header rather than the inner TCP header, so that we can (if we want to)
> increment the inner IP IDs. Of course, if we Officially Don't Care about
> inner IP IDs that's not a problem.
The inner IP IDs are the ones that are guaranteed to be the ones we
can leave fixed since TCP will require that the DF bit be set. The
current VXLAN implementation does not set DF for the outer headers.
So really we don't have too many options right now if we are wanting
to support tunnels.
> Iwonder if we could just always fill in inner_network_headereven if we're
> not doing encapsulation. Or does it end up pointing to a 'middle' header
> in the case of nested encap?
Right now neseted encap is not supported because tunnels don't
advertise hw_encap_features.
>> As far as retreating I don't really see how that would work. In most
>> cases it is an all-or-nothing proposition to setup these outer
>> headers. Either we can segment the frame with the outer headers
>> replicated or we cannot. I suspect it would end up being a common
>> case where the hardware will update the outer IP and inner TCP
>> headers, but I think the outer L4 and inner IP headers will be the
>> ones that most likely always end up being static.
> Having thought a bit more about this, I think supporting anything other
> than "hardware updates inner [IP and] TCPheaders" is needlessly complex
> (well, we still have to handle "hardware updates everything 'cos it
> thinks it knows best", because that already exists in the wild in
> hardware that might not support the new way). I don't think there's
> likely to be a case where hardware can do half of the segmentation at
> the same time as copying headers for the other half.
The ixgbe approach skips the inner IP header. The inner IPv4 ID field
can be fixed since TCP requires the DF bit be set.
> I also still don't see why hardware would want to update the outer IP
> header - can you explain?
Inner IP header has DF bit set. As such we can ignore IPv4 ID field.
The outer header will not. As such RFC6864 says we have to increment
it since it could be fragmented.
>> Also we already
>> have code paths in place in the GRE driver for instance that prevent
>> us from using GSO in the case of TUNNEL_SEQ being enabled.
> Oh good, one less thing to worry about.
>
>>> (It might still be necessary to put the true length in the TCP header, if
>>> hardware is using that as an input to segmentation. I think sfc hardware
>>> just uses 'total length of all payload DMA descriptors', but others might
>>> behave differently.)
>> That is what most drivers do. The way I kind of retained that is that
>> the TCP header doesn't include an actual length field, but I left the
>> pseudo-header using the full length of all data.
> But then you're guaranteed to have to update the outer L4 checksum when
> yousegment (because outer LCO reads the inner pseudo-header checksum).
Yes, but the LCO checksum is computed when we are constructing the
headers. I have already taken that into account in my patches.
> Why not use the single-segment length in the pseudo-header, then the
> outer L4 checksum is already the right thing? (And if yourhardware
> can't be told to leave the outer L4 checksum alone, then it's not worth
> the trouble of trying to support GSO partial for it, since it clearly
> wants to do old-style "NIC knows best" TSO.)
The problem then becomes that I needs special case code. One for
standard TSO and one for this special case. If I leave it as is I can
use the same code to cancel out the length in the TCP pseudo-header
checksum for either case.
> Then if the hardware is assuming the (inner) pseudo is using the full
> length, and is going to include that edit in its checksum calculation,
> you can just do the opposite edit in the driver, just before handing
> the packet off to the hardware.
I want to keep the scope of this change limited. By keeping the inner
TCP checksum calculation working the same way I don't have to write up
some new function.
The general idea to my approach is to treat the the UDP or GRE header
to the inner IP header as an IP header extension type value. That
will allow us to trick most hardware into being able to accept it.
> Again, the idea is that we optimise for GSO partial by making it a plain
> header copy everywhere, and put all the 'fix things up' on the _other_
> path.
That is what I went for. The only part that isn't a plain header copy
is the TCP pseudo-header checksum.
> And yes, I forgot (and keep forgetting) that the TCP header doesn't have
> an explicit length field.
Right. That is one of the other reasons for not wanting to pull that
length out since the actual length is the only real length value we
can track with the TCP header since it doesn't contain one of its own.
>> My thought was to
>> end up using something like the ixgbe approach for most devices. What
>> I did there was replicate the tunnel headers and inner IPv4 or IPv6
>> header. In the case of ixgbe and i40e I can throw away the checksum
>> and length values for the outer IP header, one thing I was curious
>> about is if I really needed to retain the full packet size for those.
> Again, the outer IP header should be computed for a single segment
> rather than for the superframe, so that it doesn't need to be edited
> later. It should be possible to send a "GSO partial" frame to TSO
> withouta single GSO callback needing to be called; similarly,
> software GSO should be able to just copy the outer headers, and only
> need to know how to update the TCP header. (See below for my "what
> a NIC should do" TSO design, which software can easily emulate.)
>>> However, I haven't yet had the time to attempt to implement this, so there
>>> might be some obvious reason I'm missing why this is impossible.
>>> Also, it's possible that I've completely misunderstood your patch and it's
>>> orthogonal to and can coexist with what I'm suggesting.
>> The one piece I could really use would be an understanding of what
>> inputs your hardware is expecting in order for us to extend TSO to
>> support this kind of approach. Then I could start tailoring the
>> output generated so that we had something that would work with more
>> devices. I was thinking the approach I have taken is fairly generic
>> since essentially it allows us to get away with TSO as long as we are
>> allowed to provide the offsets for the IP header and the TCP header.
>> From what I can tell it looks like the Solarflare drivers do something
>> similar so you might even try making changes similar to what I did for
>> ixgbe to see if you can get a proof of concept working for sfc.
> So, this is all still slightly speculative because while I've talked to
> some of our firmware developers, we haven't got as far as actually writing
> the new firmware. I'd also like to make clear that this isn't "what
> Solarflare has officially decided to do"; rather it's "what I'm currently
> trying to convince people at Solarflare to do".
> But what I think we're going to end up with is this:
>
> The kernel will give us a packet that looks like a single MSS-sized segment
> except that the payload is too long; the length fields in all the headers
> are for an MSS-sized segment, and the checksums are correct for that
> (except that the inner TCP checksum is, of course, the pseudo-header sum
> rather than a sum over the whole payload). The kernel will also tell us
> where in the packet the inner IP header begins. The driver will then give
> the following descriptors to the hardware:
> * A TSO descriptor, containing the offset of the inner IP header, and the
> MSS to use for segmentation.
> * A DMA descriptor containing all the headers (i.e. up to the end of the
> inner TCP header).
> * A series of DMA descriptors containing the payload, with a total length
> divisible by the MSS we thought of earlier.
> The NIC can now read IHL from the inner IP header, and thereby compute the
> offset of the inner TCP header, and the csum_start/offset values.
> Then for each MSS-sized block of payload, the NIC will do the following:
> * transmit header + payload block
> * increment inner IP ID, and decrement inner IP checksum (ones-complement)
> * add MSS to TCP sequence number
>
> I believe this is something thatany NIC with TSO support should be able to
> learn to do, with appropriate firmware changes. It might be a while before
> there are NICs in the wild that can do this,though.
The only issue I see is the expectation that the outer headers go
untouched is problematic. The outer headers are where things like
fragmentation will occur in transit. In addition I suspect a number
of devices other than the Intel NICs probably use the network header
to determine where to insert L2 tags such as VLAN.
Like I have said with the current solution I could probably update
igb, igbvf, fm10k, ixgbe, ixgbevf, i40e, and i40evf to support this.
That covers pretty much the entire Intel series of drivers in terms of
enterprise. The question is what do I need to enable to support other
drivers. It doesn't seem like there would be too much but the bit I
would need to know is what the expectations are when computing outer
IPv4 checksums.
- Alex
^ permalink raw reply
* Re: [PATCH] macb: fix PHY reset
From: Sergei Shtylyov @ 2016-03-22 21:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nicolas.ferre
In-Reply-To: <20160322.160738.145737905213296810.davem@redhat.com>
On 03/22/2016 11:07 PM, David Miller wrote:
>> On 03/22/2016 10:27 PM, Sergei Shtylyov wrote:
>>
>>> The driver calls gpiod_set_value() with GPIOD_OUT_* instead of 0 and
>>> 1, as
>>> a result the PHY isn't really put back into reset state in
>>> macb_remove().
>>> Moreover, the driver assumes that something else has set the GPIO
>>> direction
>>> to output, so if it has not, the PHY wouldn't be taken out of reset in
>>
>> s/wouldn't/may not/, sorry. Do I need to resend?
>
> No need, I fixed it up by hand.
>
> Applied, thanks.
Oops, forgot another tag:
Fixes: 270c499f0993 ("net/macb: Update device tree binding for resetting PHY
using GPIO")
Too late probably... :-(
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
From: Cong Wang @ 2016-03-22 21:10 UTC (permalink / raw)
To: David Ahern; +Cc: Lance Richardson, Linux Kernel Network Developers
In-Reply-To: <56F1B300.8070209@cumulusnetworks.com>
On Tue, Mar 22, 2016 at 2:02 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 3/22/16 2:45 PM, Cong Wang wrote:
>>
>> @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
>> struct flowi4 *flp,
>> nh->nh_flags & RTNH_F_LINKDOWN &&
>> !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
>> continue;
>> - if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
>> + if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF))
>> {
>> if (flp->flowi4_oif &&
>> flp->flowi4_oif != nh->nh_oif)
>> continue;
>>
>> For me, it looks the bug was introduce by commit
>> 35ebf65e851c6d9731abc6362b1.
>>
>
> Arguably yes since it added the function without initializing flags.
>
> The commit I referenced (and even the VRF predecessor both of which
> originated in the v4.3) is the one introducing use of flow flags to the
> lookup. From a stable perspective going back to v4.3 is what matters.
>
Yeah, then it is commit 613d09b30f8b589d5a9b497. It doesn't matter
for backport, but matters for accuracy. ;)
^ permalink raw reply
* Re: [PATCH net-next] net: Fix remote checksum offload with GUE
From: Tom Herbert @ 2016-03-22 21:10 UTC (permalink / raw)
To: Alexander Duyck; +Cc: David Miller, Netdev, Kernel Team, Alex Duyck
In-Reply-To: <CAKgT0UfNrmmG3var3OQ04fPK08nNkip=ZC3EAKXc6PsFeYjFbA@mail.gmail.com>
On Tue, Mar 22, 2016 at 1:20 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
>> In skb_segment the check of whether or not to perform the checksum on
>> host was changed to not consider rather remote checksum offload is
>> in use. In the case that can_checksum_protocol fails the checksum
>> is computed regardless. __skb_udp_tunnel_segment was modified in a
>> related patch to add NETIF_F_HW_CSUM into features when grabbing
>> the enc_features and remote checksum offload is being done. The
>> problem is that this bit can be cleared in lower GSO layers that
>> are also doing tunneling (e.g. ipip, GRE when used with GUE),
>> so when we get to skb_segment that intent has been lost and
>> can_checksum_protocol fails.
>
> So what you are describing sounds like a tunnel in tunnel scenario.
> It might work better to just skip masking the features if
> skb->remcsum_offload is set rather than trying to change how we
> perform the offload.
>
To be clear, my patch is restoring the old behavior not implementing a new one.
> I'm pretty sure this will cause data corruption and maybe a kernel
> panic if Tx checksum offload is disabled.
>
Nope, working fine for me.
The outer checksum would be computed in
>> This patch:
>>
>> - Restores the check in skb_segment to look at remote checksum offload.
>> - Removes the code in __skb_udp_tunnel_segment to force the
>> NETIF_F_HW_CSUM feature since this is no longer useful with above
>> change.
>> - Removes check for remote checksum offload in gso_reset_checksum.
>> A special case should not be needed here.
>>
>> Tested: Single netperf STREAM over GUE-ipip
>>
>> Before fix:
>> 5625 Mbps
>> After fix:
>> 6410 Mbps
>>
>> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
>> Signed-off-by: Tom Herbert <tom@herbertland.com>
>> ---
>> include/linux/skbuff.h | 4 ----
>> net/core/skbuff.c | 5 ++---
>> net/ipv4/udp_offload.c | 10 ----------
>> 3 files changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 15d0df9..f6fe8a5 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>>
>> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
>> {
>> - /* Do not update partial checksums if remote checksum is enabled. */
>> - if (skb->remcsum_offload)
>> - return;
>> -
>> SKB_GSO_CB(skb)->csum = res;
>> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
>> }
>
> I'm pretty sure this part here will break things when you don't have
> an outer offload enabled. What NIC did you test this on? Did you try
> disabling the Tx checksum support in the hardware to see what would
> happen?
>
Mellanox.
When TX checksum is disabled the outer checksum is computed in
__skb_udp_tunnel_segment.
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index f044f97..e4eb78d 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -3259,14 +3259,13 @@ skip_fraglist:
>> nskb->truesize += nskb->data_len;
>>
>> perform_csum_check:
>> - if (!csum) {
>> + if (!csum && !nskb->remcsum_offload) {
>> if (skb_has_shared_frag(nskb)) {
>> err = __skb_linearize(nskb);
>> if (err)
>> goto err;
>> }
>> - if (!nskb->remcsum_offload)
>> - nskb->ip_summed = CHECKSUM_NONE;
>> + nskb->ip_summed = CHECKSUM_NONE;
>> SKB_GSO_CB(nskb)->csum =
>> skb_checksum(nskb, doffset,
>> nskb->len - doffset, 0);
>
> I'm pretty sure this is going to cause a huge mess if you are
> requesting remote checksum but cannot perform an outer checksum. One
> of the reasons I merged these features together the way I did was
> because we needed to perform the initial checksum to avoid causing a
> kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
> and SKB_GSO_CB()->csum_start fields populated.
>
Nope, if we can't perform outer checksum it is done in the host. I'm
not seeing any problem.
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 8a3405a..f86f1e1 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>>
>> features &= skb->dev->hw_enc_features;
>>
>> - /* The only checksum offload we care about from here on out is the
>> - * outer one so strip the existing checksum feature flags and
>> - * instead set the flag based on our outer checksum offload value.
>> - */
>> - if (remcsum || ufo) {
>> - features &= ~NETIF_F_CSUM_MASK;
>> - if (!need_csum || offload_csum)
>> - features |= NETIF_F_HW_CSUM;
>> - }
>> -
>> /* segment inner packet. */
>> segs = gso_inner_segment(skb, features);
>> if (IS_ERR_OR_NULL(segs)) {
>
> This part breaks UDP fragmentation I am pretty sure. We need this bit
> for UFO to avoid having to perform a checksum over the data twice if
> we are offloading the outer UDP checksum.
The maybe leave this for ufo if it is a problem (please test your
suppositions!), but this is not good for remcsum offload.
Tom
^ permalink raw reply
* Re: [PATCH] macb: fix PHY reset
From: Sergei Shtylyov @ 2016-03-22 21:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nicolas.ferre
In-Reply-To: <20160322.160738.145737905213296810.davem@redhat.com>
On 03/22/2016 11:07 PM, David Miller wrote:
>>> The driver calls gpiod_set_value() with GPIOD_OUT_* instead of 0 and
>>> 1, as
>>> a result the PHY isn't really put back into reset state in
>>> macb_remove().
>>> Moreover, the driver assumes that something else has set the GPIO
>>> direction
>>> to output, so if it has not, the PHY wouldn't be taken out of reset in
>>
>> s/wouldn't/may not/, sorry. Do I need to resend?
>
> No need, I fixed it up by hand.
>
> Applied, thanks.
Thank you! gpio_request() or smth of that sort wouldn't hurt too (the pin
won't be switched to the GPIO mode on the pin function controller that I have
here...
Anyway, this code is doomed if my phylib reset GPIO patch (to be posted
yet) is accepted ...
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
From: David Ahern @ 2016-03-22 21:02 UTC (permalink / raw)
To: Cong Wang; +Cc: Lance Richardson, Linux Kernel Network Developers
In-Reply-To: <CAM_iQpV6srT84xa4y0WHrKG0mbO2xs=rnKKCx1XB9FtWusxCgw@mail.gmail.com>
On 3/22/16 2:45 PM, Cong Wang wrote:
> @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
> struct flowi4 *flp,
> nh->nh_flags & RTNH_F_LINKDOWN &&
> !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
> continue;
> - if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
> + if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
> if (flp->flowi4_oif &&
> flp->flowi4_oif != nh->nh_oif)
> continue;
>
> For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1.
>
Arguably yes since it added the function without initializing flags.
The commit I referenced (and even the VRF predecessor both of which
originated in the v4.3) is the one introducing use of flow flags to the
lookup. From a stable perspective going back to v4.3 is what matters.
^ permalink raw reply
* Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
From: Cong Wang @ 2016-03-22 20:45 UTC (permalink / raw)
To: David Ahern; +Cc: Lance Richardson, Linux Kernel Network Developers
In-Reply-To: <56F18EEE.4070000@cumulusnetworks.com>
On Tue, Mar 22, 2016 at 11:29 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 3/22/16 9:31 AM, Lance Richardson wrote:
>>
>> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
>> before calling fib_lookup(), which means fib_table_lookup() is
>> using non-deterministic data at this line:
>>
>> if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>>
>> Fix by initializing fl4.flowi4_flags to zero.
>>
>> Signed-off-by: Lance Richardson <lrichard@redhat.com>
>> ---
>> net/ipv4/fib_frontend.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>> index 21add55..896844a 100644
>> --- a/net/ipv4/fib_frontend.c
>> +++ b/net/ipv4/fib_frontend.c
>> @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>> fl4.flowi4_scope = scope;
>> fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark :
>> 0;
>> fl4.flowi4_tun_key.tun_id = 0;
>> + fl4.flowi4_flags = 0;
>> if (!fib_lookup(net, &fl4, &res, 0))
>> return FIB_RES_PREFSRC(net, res);
>> } else {
>>
>
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
Why does this patch fix this commit? Even before this commit, flowi4_flags
was already tested for other bit:
@@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
struct flowi4 *flp,
nh->nh_flags & RTNH_F_LINKDOWN &&
!(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
continue;
- if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
+ if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
if (flp->flowi4_oif &&
flp->flowi4_oif != nh->nh_oif)
continue;
For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1.
^ permalink raw reply
* Re: [PATCH net-next] net: Fix remote checksum offload with GUE
From: Alexander Duyck @ 2016-03-22 20:20 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Netdev, Kernel Team, Alex Duyck
In-Reply-To: <1458674365-3944200-1-git-send-email-tom@herbertland.com>
On Tue, Mar 22, 2016 at 12:19 PM, Tom Herbert <tom@herbertland.com> wrote:
> In skb_segment the check of whether or not to perform the checksum on
> host was changed to not consider rather remote checksum offload is
> in use. In the case that can_checksum_protocol fails the checksum
> is computed regardless. __skb_udp_tunnel_segment was modified in a
> related patch to add NETIF_F_HW_CSUM into features when grabbing
> the enc_features and remote checksum offload is being done. The
> problem is that this bit can be cleared in lower GSO layers that
> are also doing tunneling (e.g. ipip, GRE when used with GUE),
> so when we get to skb_segment that intent has been lost and
> can_checksum_protocol fails.
So what you are describing sounds like a tunnel in tunnel scenario.
It might work better to just skip masking the features if
skb->remcsum_offload is set rather than trying to change how we
perform the offload.
I'm pretty sure this will cause data corruption and maybe a kernel
panic if Tx checksum offload is disabled.
> This patch:
>
> - Restores the check in skb_segment to look at remote checksum offload.
> - Removes the code in __skb_udp_tunnel_segment to force the
> NETIF_F_HW_CSUM feature since this is no longer useful with above
> change.
> - Removes check for remote checksum offload in gso_reset_checksum.
> A special case should not be needed here.
>
> Tested: Single netperf STREAM over GUE-ipip
>
> Before fix:
> 5625 Mbps
> After fix:
> 6410 Mbps
>
> Fixes: 76443456227097179c1482 ("net: Move GSO csum into SKB_GSO_CB")
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---
> include/linux/skbuff.h | 4 ----
> net/core/skbuff.c | 5 ++---
> net/ipv4/udp_offload.c | 10 ----------
> 3 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 15d0df9..f6fe8a5 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3615,10 +3615,6 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>
> static inline void gso_reset_checksum(struct sk_buff *skb, __wsum res)
> {
> - /* Do not update partial checksums if remote checksum is enabled. */
> - if (skb->remcsum_offload)
> - return;
> -
> SKB_GSO_CB(skb)->csum = res;
> SKB_GSO_CB(skb)->csum_start = skb_checksum_start(skb) - skb->head;
> }
I'm pretty sure this part here will break things when you don't have
an outer offload enabled. What NIC did you test this on? Did you try
disabling the Tx checksum support in the hardware to see what would
happen?
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index f044f97..e4eb78d 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3259,14 +3259,13 @@ skip_fraglist:
> nskb->truesize += nskb->data_len;
>
> perform_csum_check:
> - if (!csum) {
> + if (!csum && !nskb->remcsum_offload) {
> if (skb_has_shared_frag(nskb)) {
> err = __skb_linearize(nskb);
> if (err)
> goto err;
> }
> - if (!nskb->remcsum_offload)
> - nskb->ip_summed = CHECKSUM_NONE;
> + nskb->ip_summed = CHECKSUM_NONE;
> SKB_GSO_CB(nskb)->csum =
> skb_checksum(nskb, doffset,
> nskb->len - doffset, 0);
I'm pretty sure this is going to cause a huge mess if you are
requesting remote checksum but cannot perform an outer checksum. One
of the reasons I merged these features together the way I did was
because we needed to perform the initial checksum to avoid causing a
kernel panic later on. Otherwise we don't have the SKB_GSO_CB()->csum
and SKB_GSO_CB()->csum_start fields populated.
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 8a3405a..f86f1e1 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -78,16 +78,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>
> features &= skb->dev->hw_enc_features;
>
> - /* The only checksum offload we care about from here on out is the
> - * outer one so strip the existing checksum feature flags and
> - * instead set the flag based on our outer checksum offload value.
> - */
> - if (remcsum || ufo) {
> - features &= ~NETIF_F_CSUM_MASK;
> - if (!need_csum || offload_csum)
> - features |= NETIF_F_HW_CSUM;
> - }
> -
> /* segment inner packet. */
> segs = gso_inner_segment(skb, features);
> if (IS_ERR_OR_NULL(segs)) {
This part breaks UDP fragmentation I am pretty sure. We need this bit
for UFO to avoid having to perform a checksum over the data twice if
we are offloading the outer UDP checksum.
^ permalink raw reply
* Re: [PATCH v3 0/2] AF_VSOCK: Shrink the area influenced by prepare_to_wait
From: David Miller @ 2016-03-22 20:19 UTC (permalink / raw)
To: imbrenda; +Cc: labbott, netdev, linux-kernel
In-Reply-To: <1458662752-2415-1-git-send-email-imbrenda@linux.vnet.ibm.com>
From: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com>
Date: Tue, 22 Mar 2016 17:05:50 +0100
> This patchset applies on net-next.
>
> I think I found a problem with the patch submitted by Laura Abbott
> ( https://lkml.org/lkml/2016/2/4/711 ): we might miss wakeups.
> Since the condition is not checked between the prepare_to_wait and the
> schedule(), if a wakeup happens after the condition is checked but before
> the sleep happens, and we miss it. ( A description of the problem can be
> found here: http://www.makelinux.net/ldd3/chp-6-sect-2 ).
>
> The first patch reverts the previous broken patch, while the second patch
> properly fixes the sleep-while-waiting issue.
Series applied, thanks for following up on this.
^ permalink raw reply
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: David Miller @ 2016-03-22 20:17 UTC (permalink / raw)
To: jesse; +Cc: ecree, alexander.duyck, aduyck, netdev, tom
In-Reply-To: <CAEh+42jq1byHx0yuZgUCLqfk3G3eGfhoA1oq4Wwn5SmefCbNnQ@mail.gmail.com>
From: Jesse Gross <jesse@kernel.org>
Date: Tue, 22 Mar 2016 13:11:21 -0700
> Features that have been designed this way in the past are usually
> pretty fragile. Not only would you have to track changes in the
> routing table but you could have bridges, tc, vlan devices, etc. all
> of which might change the path of the packet and would have to somehow
> propagate this information. It's much more robust to resolve the
> device capabilities just before you hand the packet to that device.
> Plus, anything along the path of the packet (iptables, for example)
> that looks at the headers might potentially need to be aware of this
> optimization.
Indeed, this is a major fundamental issue in our stack right now. I
keep being reminded of that ugly change we had to make to accomodate
scatter-gather limitations for Infiniband devices, exactly because
properties don't propagate properly through all of the layers right
now.
But we have to solve this somehow, the packetizer has to know certain
basic properties of the ultimate device in order to function properly.
This requirement is unavoidable.
^ permalink raw reply
* Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
From: Eric Dumazet @ 2016-03-22 20:15 UTC (permalink / raw)
To: David Ahern; +Cc: Lance Richardson, netdev
In-Reply-To: <56F19660.4040502@cumulusnetworks.com>
On Tue, 2016-03-22 at 13:00 -0600, David Ahern wrote:
> On 3/22/16 12:56 PM, Lance Richardson wrote:
> >
> > Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
> DaveM: this should go into stable releases back to v4.3.
>
The 'Fixes' tag tells this already ;)
$ git describe --contains 58189ca7b2741
v4.3-rc3~13^2~63
^ permalink raw reply
* Re: [PATCH] ipv6: Fix the pmtu path for connected UDP socket
From: Cong Wang @ 2016-03-22 20:13 UTC (permalink / raw)
To: Wei Wang
Cc: Martin KaFai Lau, Wei Wang, Eric Dumazet, David Miller,
Eric Dumazet, Linux Kernel Network Developers
In-Reply-To: <CAC15z3jDRvJR4uZ97mC+beaMaPQvAvTHB9OLKCdQ4hcwRu07vw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 429 bytes --]
On Tue, Mar 22, 2016 at 11:03 AM, Wei Wang <tracywwnj@gmail.com> wrote:
> Thanks Martin and Cong.
>
> I guess then we are going with the following fix in ip6_sk_update_pmtu():
> 1. call ip6_upate_pmtu() as it is
> 2. do a dst_check()
> 3. re-lookup() if it is invalid
> 4. and then do a ip6_dst_store()/dst_set
Exactly, please try the attached patch. Note I did nothing more than a
compile test.
Does it make sense to you now?
[-- Attachment #2: ipv6-route-pmtu.diff --]
[-- Type: text/plain, Size: 3368 bytes --]
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed44663..fcea05e 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1346,18 +1346,20 @@ static bool rt6_cache_allowed_for_pmtu(const struct rt6_info *rt)
(rt->rt6i_flags & RTF_PCPU || rt->rt6i_node);
}
-static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
- const struct ipv6hdr *iph, u32 mtu)
+static struct dst_entry* __ip6_rt_update_pmtu(struct dst_entry *dst,
+ const struct sock *sk,
+ const struct ipv6hdr *iph,
+ u32 mtu, bool hold)
{
struct rt6_info *rt6 = (struct rt6_info *)dst;
if (rt6->rt6i_flags & RTF_LOCAL)
- return;
+ return dst;
dst_confirm(dst);
mtu = max_t(u32, mtu, IPV6_MIN_MTU);
if (mtu >= dst_mtu(dst))
- return;
+ return dst;
if (!rt6_cache_allowed_for_pmtu(rt6)) {
rt6_do_update_pmtu(rt6, mtu);
@@ -1372,11 +1374,13 @@ static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
daddr = &sk->sk_v6_daddr;
saddr = &inet6_sk(sk)->saddr;
} else {
- return;
+ return dst;
}
nrt6 = ip6_rt_cache_alloc(rt6, daddr, saddr);
if (nrt6) {
rt6_do_update_pmtu(nrt6, mtu);
+ if (hold)
+ dst_hold(&nrt6->dst);
/* ip6_ins_rt(nrt6) will bump the
* rt6->rt6i_node->fn_sernum
@@ -1384,14 +1388,17 @@ static void __ip6_rt_update_pmtu(struct dst_entry *dst, const struct sock *sk,
* invalidate the sk->sk_dst_cache.
*/
ip6_ins_rt(nrt6);
+ return &nrt6->dst;
}
}
+
+ return dst;
}
static void ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
struct sk_buff *skb, u32 mtu)
{
- __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu);
+ __ip6_rt_update_pmtu(dst, sk, skb ? ipv6_hdr(skb) : NULL, mtu, false);
}
void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
@@ -1410,15 +1417,52 @@ void ip6_update_pmtu(struct sk_buff *skb, struct net *net, __be32 mtu,
dst = ip6_route_output(net, NULL, &fl6);
if (!dst->error)
- __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu));
+ __ip6_rt_update_pmtu(dst, NULL, iph, ntohl(mtu), false);
dst_release(dst);
}
EXPORT_SYMBOL_GPL(ip6_update_pmtu);
void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
{
- ip6_update_pmtu(skb, sock_net(sk), mtu,
- sk->sk_bound_dev_if, sk->sk_mark);
+ const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
+ struct net *net = sock_net(sk);
+ struct dst_entry *ndst, *dst;
+ struct flowi6 fl6;
+
+ memset(&fl6, 0, sizeof(fl6));
+
+ bh_lock_sock(sk);
+
+ fl6.flowi6_oif = sk->sk_bound_dev_if;
+ fl6.flowi6_mark = sk->sk_mark ? : IP6_REPLY_MARK(net, skb->mark);
+ fl6.daddr = iph->daddr;
+ fl6.saddr = iph->saddr;
+ fl6.flowlabel = ip6_flowinfo(iph);
+
+ dst = sk_dst_get(sk);
+ if (sock_owned_by_user(sk) || !dst) {
+ ip6_update_pmtu(skb, net, mtu, fl6.flowi6_oif, fl6.flowi6_mark);
+ goto out;
+ }
+
+ if (dst->obsolete && !dst->ops->check(dst, 0)) {
+ dst_release(dst);
+ dst = ip6_route_output(net, sk, &fl6);
+ if (dst->error)
+ goto out;
+ }
+
+ ndst = __ip6_rt_update_pmtu(dst, sk, iph, ntohl(mtu), true);
+ if (ndst != dst) {
+ dst_release(dst);
+ dst = ndst;
+ }
+
+ if (sk->sk_state == TCP_ESTABLISHED)
+ ip6_dst_store(sk, dst, &iph->daddr, &iph->saddr);
+out:
+ bh_unlock_sock(sk);
+ dst_release(dst);
}
EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
^ permalink raw reply related
* Re: [RFC PATCH 7/9] GSO: Support partial segmentation offload
From: Jesse Gross @ 2016-03-22 20:11 UTC (permalink / raw)
To: Edward Cree
Cc: Alexander Duyck, Alexander Duyck, Netdev, David Miller,
Tom Herbert
In-Reply-To: <56F19FAA.3090005@solarflare.com>
On Tue, Mar 22, 2016 at 12:40 PM, Edward Cree <ecree@solarflare.com> wrote:
> On 22/03/16 17:47, Alexander Duyck wrote:
>> On Tue, Mar 22, 2016 at 10:00 AM, Edward Cree <ecree@solarflare.com> wrote:
>>> On 18/03/16 23:25, Alexander Duyck wrote:
>>>> This patch adds support for something I am referring to as GSO partial.
>>>> The basic idea is that we can support a broader range of devices for
>>>> segmentation if we use fixed outer headers and have the hardware only
>>>> really deal with segmenting the inner header. The idea behind the naming
>>>> is due to the fact that everything before csum_start will be fixed headers,
>>>> and everything after will be the region that is handled by hardware.
>>>>
>>>> With the current implementation it allows us to add support for the
>>>> following GSO types with an inner TSO or TSO6 offload:
>>>> NETIF_F_GSO_GRE
>>>> NETIF_F_GSO_GRE_CSUM
>>>> NETIF_F_UDP_TUNNEL
>>>> NETIF_F_UDP_TUNNEL_CSUM
>>>>
>>>> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
>>>> ---
>>> If I'm correctly understanding what you're doing, you're building a large
>>> TCP segment, feeding it through the encapsulation drivers as normal, then
>>> at GSO time you're fixing up length fields, checksums etc. in the headers.
>>> I think we can do this more simply, by making it so that at the time when
>>> we _generate_ the TCP segment, we give it headers saying it's one MSS big,
>>> but have several MSS of data. Similarly when adding the encap headers,
>>> they all need to get their lengths from what the layer below tells them,
>>> rather than the current length of data in the SKB. Then at GSO time all
>>> the headers already have the right things in, and you don't need to call
>>> any per-protocol GSO callbacks for them.
>> One issue I have to deal with here is that we have no way of knowing
>> what the underlying hardware can support at the time of segment being
>> created. You have to keep in mind that what we have access to is the
>> tunnel dev in many cases, not the underlying dev so we don't know if
>> things can be offloaded to hardware or not. By pushing this logic
>> into the GSO code we can actually implement it without much overhead
>> since we either segment it into an MSS multiple, or into single MSS
>> sized chunks. This way we defer the decision until the very last
>> moment when we actually know if we can offload some portion of this in
>> hardware or not.
> But won't the tunnel dev have the feature flag for GSO_PARTIAL depending
> on what the underlying dev advertises? (Or, at least, could we make it
> bethatway?)
Features that have been designed this way in the past are usually
pretty fragile. Not only would you have to track changes in the
routing table but you could have bridges, tc, vlan devices, etc. all
of which might change the path of the packet and would have to somehow
propagate this information. It's much more robust to resolve the
device capabilities just before you hand the packet to that device.
Plus, anything along the path of the packet (iptables, for example)
that looks at the headers might potentially need to be aware of this
optimization.
You're also assuming that the generating TCP stack is resident on the
same machine as the device that does the offloads. That's not
necessarily true in the case of VMs or remote senders whose packets
have been GRO'ed.
Keeping the core stack consistent and just handling this at the
GRO/driver layer as Alex has here seems preferable to me.
^ permalink raw reply
* Re: [PATCH] macb: fix PHY reset
From: David Miller @ 2016-03-22 20:07 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: netdev, nicolas.ferre
In-Reply-To: <56F19E2D.8000700@cogentembedded.com>
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Tue, 22 Mar 2016 22:34:05 +0300
> On 03/22/2016 10:27 PM, Sergei Shtylyov wrote:
>
>> The driver calls gpiod_set_value() with GPIOD_OUT_* instead of 0 and
>> 1, as
>> a result the PHY isn't really put back into reset state in
>> macb_remove().
>> Moreover, the driver assumes that something else has set the GPIO
>> direction
>> to output, so if it has not, the PHY wouldn't be taken out of reset in
>
> s/wouldn't/may not/, sorry. Do I need to resend?
No need, I fixed it up by hand.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
From: David Miller @ 2016-03-22 20:03 UTC (permalink / raw)
To: lrichard; +Cc: netdev, dsa
In-Reply-To: <688989936.72666829.1458673139415.JavaMail.zimbra@redhat.com>
From: Lance Richardson <lrichard@redhat.com>
Date: Tue, 22 Mar 2016 14:58:59 -0400 (EDT)
> Apologies, that should have been [PATCH v2 net].
No worries.
Applied and queued up for -stable, thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox