* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: David Miller @ 2017-09-16 3:56 UTC (permalink / raw)
To: eric.dumazet; +Cc: f.fainelli, rosenp, netdev, zajec5, nbd
In-Reply-To: <1505520210.29839.7.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 Sep 2017 17:03:30 -0700
> On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
>> On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com>
>> wrote:
>> >On a linksys E1200v1 (actually a crossflashed E1000v2), the
>> offloading
>> >features give no measurable benefit to speed or latency. Furthermore,
>> >disabling GRO actually improves iperf performance by a whoppimg
>> 3mbps.
>>
>> Do you have a way to generate gigabit tests and see what results you
>> are getting? We probably are not going to see a 30% improvement just
>> by extrapolation.
>>
> +1
>
> It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
> driver to handle these features.
>
> And of course GRO was not removed, meaning the bench results were non
> conclusive.
My sentiments exactly, I think this is a completely unwise change.
^ permalink raw reply
* Re: [PATCH net] tcp: fix data delivery rate
From: Soheil Hassas Yeganeh @ 2017-09-16 1:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, liujian, Eric Dumazet, Yuchung Cheng, Jerry Chu,
netdev, weiyongjun (A), wangkefeng 00227729
In-Reply-To: <1505519262.29839.5.camel@edumazet-glaptop3.roam.corp.google.com>
On Fri, Sep 15, 2017 at 7:47 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Now skb->mstamp_skb is updated later, we also need to call
> tcp_rate_skb_sent() after the update is done.
>
> Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Nice catch!
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1505522096.29839.13.camel@edumazet-glaptop3.roam.corp.google.com>
I have not. Unfortunately I own no gigabit hardware to test this on.
The MIPS CPU runs at 300MHz on my unit.
On Fri, 2017-09-15 at 17:34 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:23 -0700, Rosen Penev wrote:
> > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> > offloading features give no measurable benefit to speed or latency.
> > Furthermore, disabling GRO actually improves iperf performance by a
> > whoppimg 3mbps. Results:
> >
> > Currently:
> >
> > v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an
> > identical result.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> > drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c
> > b/drivers/net/ethernet/broadcom/bgmac.c
> > index 48d672b204a4..1fb0053aeee7 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> > @@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac
> > *bgmac, struct bgmac_dma_ring *ring,
> > skb->protocol = eth_type_trans(skb, bgmac-
> > >net_dev);
> > bgmac->net_dev->stats.rx_bytes += len;
> > bgmac->net_dev->stats.rx_packets++;
> > - napi_gro_receive(&bgmac->napi, skb);
> > + netif_receive_skb(skb);
> > handled++;
> > } while (0);
> >
>
> And have you tested 1Gbit link speed ?
> ( Or 2.5 Gbit link speed )
>
> If you want to disable GRO on your host, fine : you can use ethtool
> -K
>
>
>
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:34 UTC (permalink / raw)
To: Rosen Penev; +Cc: netdev
In-Reply-To: <20170916002305.2794-1-rosenp@gmail.com>
On Fri, 2017-09-15 at 17:23 -0700, Rosen Penev wrote:
> On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
>
> Currently:
>
> v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an identical result.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 48d672b204a4..1fb0053aeee7 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
> skb->protocol = eth_type_trans(skb, bgmac->net_dev);
> bgmac->net_dev->stats.rx_bytes += len;
> bgmac->net_dev->stats.rx_packets++;
> - napi_gro_receive(&bgmac->napi, skb);
> + netif_receive_skb(skb);
> handled++;
> } while (0);
>
And have you tested 1Gbit link speed ?
( Or 2.5 Gbit link speed )
If you want to disable GRO on your host, fine : you can use ethtool -K
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Denys Fedoryshchenko @ 2017-09-16 0:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: rosenp, Florian Fainelli, netdev, zajec5, nbd, netdev-owner
In-Reply-To: <1505521095.29839.11.camel@edumazet-glaptop3.roam.corp.google.com>
On 2017-09-16 03:18, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
>> Ok fair enough. Will only disable GRO in the driver.
>
> Well, do not even try.
>
> NETIF_F_SOFT_FEATURES is set by core networking stack in
> register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
> )
>
> Absolutely no driver disables GRO (excepts the ones playing with LRO)
I believe also iperf is definitely inconclusive test.
Except iperf there is lot of different workloads and configurations,
that might have different results.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Fainelli, netdev, zajec5, nbd
In-Reply-To: <1505521095.29839.11.camel@edumazet-glaptop3.roam.corp.google.com>
netif_receive_skb seems to have the same effect as "ethtool -K eth0 gro
off".
On Fri, 2017-09-15 at 17:18 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
> > Ok fair enough. Will only disable GRO in the driver.
>
> Well, do not even try.
>
> NETIF_F_SOFT_FEATURES is set by core networking stack in
> register_netdevice(), ( commit
> 212b573f5552c60265da721ff9ce32e3462a2cdd
> )
>
> Absolutely no driver disables GRO (excepts the ones playing with LRO)
>
>
>
>
>
>
^ permalink raw reply
* [PATCH] bgmac: Remove all offloading features, including GRO.
From: Rosen Penev @ 2017-09-16 0:23 UTC (permalink / raw)
To: netdev; +Cc: Rosen Penev
On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
Currently:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52410
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 641 75:181:12:1:1:0:0:371
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52412
[ 5] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 629 51:194:13:1:0:1:0:369
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52414
[ 4] 0.00-10.02 sec 51.9 MBytes 43.4 Mbits/sec 695 126:203:1:0:0:0:2:363
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52416
[ 5] 0.00-10.01 sec 52.4 MBytes 43.9 Mbits/sec 626 57:186:10:0:0:0:0:373
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52420
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 605 36:179:16:1:0:1:0:372
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52422
After disabling everything - including GRO:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52440
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.01 sec 55.1 MBytes 46.2 Mbits/sec 672 180:82:0:0:1:0:0:409
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52442
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 636 117:96:0:0:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52444
[ 4] 0.00-10.01 sec 55.4 MBytes 46.4 Mbits/sec 675 172:92:0:0:1:0:0:410
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52446
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 633 119:90:0:1:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52448
[ 4] 0.00-10.01 sec 55.2 MBytes 46.3 Mbits/sec 688 157:123:0:0:2:0:0:406
v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an identical result.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 48d672b204a4..1fb0053aeee7 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
skb->protocol = eth_type_trans(skb, bgmac->net_dev);
bgmac->net_dev->stats.rx_bytes += len;
bgmac->net_dev->stats.rx_packets++;
- napi_gro_receive(&bgmac->napi, skb);
+ netif_receive_skb(skb);
handled++;
} while (0);
--
2.13.5
^ permalink raw reply related
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:18 UTC (permalink / raw)
To: rosenp; +Cc: Florian Fainelli, netdev, zajec5, nbd
In-Reply-To: <1505520638.8507.2.camel@gmail.com>
On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
> Ok fair enough. Will only disable GRO in the driver.
Well, do not even try.
NETIF_F_SOFT_FEATURES is set by core networking stack in
register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
)
Absolutely no driver disables GRO (excepts the ones playing with LRO)
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:10 UTC (permalink / raw)
To: Eric Dumazet, Florian Fainelli; +Cc: netdev, zajec5, nbd
In-Reply-To: <1505520210.29839.7.camel@edumazet-glaptop3.roam.corp.google.com>
Ok fair enough. Will only disable GRO in the driver.
Tests were done using "ethtool -K eth0 gro off" and on.
On Fri, 2017-09-15 at 17:03 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
> > On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com
> > >
> > wrote:
> > > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> >
> > offloading
> > > features give no measurable benefit to speed or latency.
> > > Furthermore,
> > > disabling GRO actually improves iperf performance by a whoppimg
> >
> > 3mbps.
> >
> > Do you have a way to generate gigabit tests and see what results
> > you
> > are getting? We probably are not going to see a 30% improvement
> > just
> > by extrapolation.
> >
>
> +1
>
> It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
> driver to handle these features.
>
> And of course GRO was not removed, meaning the bench results were non
> conclusive.
>
>
>
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:03 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Rosen Penev, netdev, zajec5, nbd
In-Reply-To: <252FFED5-906E-4D1B-9E87-679D0CE801D4@gmail.com>
On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
> On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com>
> wrote:
> >On a linksys E1200v1 (actually a crossflashed E1000v2), the
> offloading
> >features give no measurable benefit to speed or latency. Furthermore,
> >disabling GRO actually improves iperf performance by a whoppimg
> 3mbps.
>
> Do you have a way to generate gigabit tests and see what results you
> are getting? We probably are not going to see a 30% improvement just
> by extrapolation.
>
+1
It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
driver to handle these features.
And of course GRO was not removed, meaning the bench results were non
conclusive.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-15 23:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, zajec5, nbd
In-Reply-To: <20170915.161403.2131524326226213839.davem@davemloft.net>
you're absolutely correct. will send an updated version shortly.
On Fri, 2017-09-15 at 16:14 -0700, David Miller wrote:
> From: Rosen Penev <rosenp@gmail.com>
> Date: Fri, 15 Sep 2017 15:22:18 -0700
>
> > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> offloading features give no measurable benefit to speed or latency.
> Furthermore, disabling GRO actually improves iperf performance by a
> whoppimg 3mbps. Results:
> ...
> > - net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM;
> > + net_dev->features &= NETIF_F_GRO;
> > net_dev->hw_features = net_dev->features;
>
> This doesn't disable GRO.
^ permalink raw reply
* [PATCH net] tcp: fix data delivery rate
From: Eric Dumazet @ 2017-09-15 23:47 UTC (permalink / raw)
To: David Miller
Cc: liujian56, edumazet, ycheng, hkchu, netdev, weiyongjun1,
wangkefeng.wang
In-Reply-To: <1505517411.29839.1.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Now skb->mstamp_skb is updated later, we also need to call
tcp_rate_skb_sent() after the update is done.
Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_output.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a85a8c2948e54b931f8cd956aa7938f7efd355bd..1c839c99114cd22bbfbd181cf702acccb3aeb61b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1002,8 +1002,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
if (clone_it) {
TCP_SKB_CB(skb)->tx.in_flight = TCP_SKB_CB(skb)->end_seq
- tp->snd_una;
- tcp_rate_skb_sent(sk, skb);
-
oskb = skb;
if (unlikely(skb_cloned(skb)))
skb = pskb_copy(skb, gfp_mask);
@@ -1128,9 +1126,10 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
tcp_enter_cwr(sk);
err = net_xmit_eval(err);
}
- if (!err && oskb)
+ if (!err && oskb) {
oskb->skb_mstamp = tp->tcp_mstamp;
-
+ tcp_rate_skb_sent(sk, oskb);
+ }
return err;
}
^ permalink raw reply related
* Re: Use after free in __dst_destroy_metrics_generic
From: Subash Abhinov Kasiviswanathan @ 2017-09-15 23:20 UTC (permalink / raw)
To: Julian Anastasov
Cc: Eric Dumazet, Cong Wang, Linux Kernel Network Developers,
Lorenzo Colitti
In-Reply-To: <alpine.LFD.2.20.1709160103190.1770@ja.home.ssi.bg>
> May be I'm missing some posting but I don't see if
> the patch was tested successfully.
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
Hi Julian
I've had this patch being tested for the last 3-4 days in our regression
rack
and I haven't seen the same issue being reproduced or even a related
crash
or leak in dst.
The original issue was reported only once to us from the regression rack
only
so the exact steps to reproduce is unknown.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH net] tcp: update skb->skb_mstamp more carefully
From: Eric Dumazet @ 2017-09-15 23:16 UTC (permalink / raw)
To: David Miller
Cc: liujian56, edumazet, ycheng, hkchu, netdev, weiyongjun1,
wangkefeng.wang
In-Reply-To: <20170915.143706.992734259378949259.davem@davemloft.net>
On Fri, 2017-09-15 at 14:37 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 13 Sep 2017 20:30:39 -0700
>
> > From: Eric Dumazet <edumazet@googl.com>
> >
> > liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> > in tcp_probe_timer() :
> > https://www.spinics.net/lists/netdev/msg454496.html
> >
> > After investigations, the root cause of the problem is that we update
> > skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> > clone or copy of it failed. One reason being a routing problem.
> >
> > This patch prevents this, solving liujian issue.
> >
> > It also removes a potential RTT miscalculation, since
> > __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> > TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> > been changed.
> >
> > A future ACK would then lead to a very small RTT sample and min_rtt
> > would then be lowered to this too small value.
> >
> > Tested:
> ...
> > Signed-off-by: Eric Dumazet <edumazet@googl.com>
> > Reported-by: liujian <liujian56@huawei.com>
>
> Applied and queued up for -stable, thanks.
Thanks David.
It seems I forgot to move the call to tcp_rate_skb_sent(sk, skb),
as this function depended on skb->skb_mstamp being up to date.
I will send a fix, sorry for not catching this earlier.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: David Miller @ 2017-09-15 23:14 UTC (permalink / raw)
To: rosenp; +Cc: netdev, zajec5, nbd
In-Reply-To: <20170915222218.6613-1-rosenp@gmail.com>
From: Rosen Penev <rosenp@gmail.com>
Date: Fri, 15 Sep 2017 15:22:18 -0700
> On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
...
> - net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> + net_dev->features &= NETIF_F_GRO;
> net_dev->hw_features = net_dev->features;
This doesn't disable GRO.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Florian Fainelli @ 2017-09-15 22:54 UTC (permalink / raw)
To: Rosen Penev, netdev, zajec5, nbd
In-Reply-To: <20170915222218.6613-1-rosenp@gmail.com>
On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com> wrote:
>On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading
>features give no measurable benefit to speed or latency. Furthermore,
>disabling GRO actually improves iperf performance by a whoppimg 3mbps.
Do you have a way to generate gigabit tests and see what results you are getting? We probably are not going to see a 30% improvement just by extrapolation.
--
Florian
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 22:41 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, Edward Cree
Cc: Shuah Khan, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <59BC2069.8050100@iogearbox.net>
On 09/15/2017 12:48 PM, Daniel Borkmann wrote:
> On 09/15/2017 08:23 PM, Daniel Borkmann wrote:
>> On 09/15/2017 08:07 PM, Alexei Starovoitov wrote:
>>> On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
>>>> On 15/09/17 17:02, Alexei Starovoitov wrote:
>>>>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>>>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>>>>> not be met on majority of the systems. Is this a hard dependency??
>>>>> It is a hard dependency and clang should be present on majority of the systems.
>>>> I think this is the wrong approach. Making kselftest hard-require clang doesn't
>>>> mean that the bpf tests will be run more often, it means that the rest of the
>>>> kselftests will be run less often. clang is quite big (when I tried to install
>>>> it on one of my test servers, I didn't have enough disk space & had to go on a
>>>> clear-out of unused packages), and most people aren't interested in the bpf
>>>> subsystem specifically; they would rather be able to skip those tests.
>>>> I feel that as long as they know they are skipping some tests (so e.g. they
>>>> won't consider it a sufficient test of a kselftest refactor), that's fine.
>>>> It's not even as though all of the bpf tests require clang; the (smaller) tests
>>>> written directly in raw eBPF instructions could still be run on such a system.
>>>> So I think we should attempt to run as much as possible but accept that clang
>>>> may not be available and have an option to skip some tests in that case.
>>>
>>> imo the value of selftests/bpf is twofold:
>>> 1. it helps bpf developers avoid regressions
>>> 2. as part of continuous integration it helps to catch bpf regressions
>>> that were somehow caused by changes in other parts of the kernel
>>>
>>> If a developer didn't bother to satisfy all bpf tests dependencies
>>> (which includes clang) and ran all tests before sending a patch,
>>> I don't want to see such patches. It just wastes maintainers time
>>> to review code and spot bugs that could have been caught by tests.
>>> Collectively we invested years of work into these tests and
>>> developers better take advantage of it by running all.
>>
>> +1
>>
>>> If a CI server didn't satisfy all bpf test dependencies,
>>> I don't want such CI setup to be running and reporting results,
>>> since it will give false sense of test coverage.
>>> Test failures due to missing dependencies are hard failures.
>>> We cannot skip them.
>>
>> +1
>
> Btw, on that note, the folks from zero-day bot run the BPF kselftests
> for a while now just fine and they do run them together with clang,
> so they have the full, proper coverage how it should be. It's not
> how it used to be in the early days, you can just go and install
> llvm/clang package on all the major distros today and you get the
> bpf target by default enabled already.
>
>>> I'd like generic XDP tests to be added to selftests/bpf which
>>> would mean that the latest iproute2 will become a hard dependency
>>> and bpf developers and CI host owners would need to upgrade
>>> their iproute2.
>>> The tests either pass or fail. Skipping them due to missing
>>> dependencies is the same as fail and in that sense I don't want
>>> to change selftests/bpf/Makefile to make it skip clang.
>>
>> I fully agree that for the BPF selftests it is very desirable
>> to not only test the verifier with couple of BPF insn snippets,
>> but to actually load and run programs that more closely resemble
>> real world programs. For more complex interactions these snippets
>> are just limited, think of tail calls, testing perf event output
>> helper, etc, which would all require to write these tests with
>> restricted C when we add them (unless we want to make writing
>> these tests a real pain ;) in which case no-one will bother to
>> write tests at all for them). Mid to long term I would definitely
>> like to see more programs in BPF selftests (e.g. moved over from
>> samples/bpf/) to increase the test coverage.
>
As I said in my earlier email:
Unless users choose to install clang, bpf will always fail run without
clang. So clang dependency is an issue for bpf test coverage in general.
That is your choice as to whether you want to increase the scope of
regression test coverage for bpf or not.
I fully understand you have weigh that against ease of writing tests.
We can leave things the way they are since:
- You can't force users to install clang and run bpf test. Users might
opt for letting bpf test fail due to unmet dependency.
- You have reasons to continue use clang and you have been using it for
a longtime.
I will try to see why make ksefltest fails on bpf even with my patch
series that addresses the source directory issue. All other tests build
and run. It is not an issue with bpf specifically, it is something that
has never been tested in this use-case.
thanks,
-- Shuah
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Julian Anastasov @ 2017-09-15 22:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, Subash Abhinov Kasiviswanathan,
Linux Kernel Network Developers, Lorenzo Colitti
In-Reply-To: <1505509219.16139.6.camel@edumazet-glaptop3.roam.corp.google.com>
Hello,
On Fri, 15 Sep 2017, Eric Dumazet wrote:
> On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> > <subashab@codeaurora.org> wrote:
> > > We are seeing a possible use after free in ip6_dst_destroy.
> > >
> > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> > > and allocated
> > > to ion driver. ion driver has also freed it. Finally the memory is freed by
> > > the
> > > fib gc and crashes since it is already deallocated.
> >
> > Does the attach (compile-only) patch help anything?
> >
> > From my _quick_ glance, it seems we miss the refcnt'ing
> > right in __dst_destroy_metrics_generic().
> >
> > Thanks!
>
>
> Hi Cong
>
> I believe your patch makes a lot of sense, please submit it formally ?
Cong's patch is wrong for few reasons:
- it will stop to kfree non-refcounted metrics
- report was for IPV6 and we set DST_METRICS_REFCOUNTED only
for IPv4, for DST_METRICS_READ_ONLY metrics
- __dst_destroy_metrics_generic is called for val without
DST_METRICS_READ_ONLY flag and such metrics are not with
DST_METRICS_REFCOUNTED flag
- ->cow_metrics and dst_cow_metrics_generic are called with
DST_METRICS_READ_ONLY flag set, there is no chance to write
new value twice, especially to write DST_METRICS_REFCOUNTED flag
and later to see this flag in __dst_destroy_metrics_generic
So, I'm not sure where exactly is the bug with the
metrics.
May be I'm missing some posting but I don't see if
the patch was tested successfully.
Regards
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 22:32 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Shuah Khan, Daniel Borkmann, Thomas Meyer, linux-kernel,
linux-kselftest, Networking, Shuah Khan
In-Reply-To: <20170915181440.adkgyqbt4yxrnbw7@ast-mbp>
On 09/15/2017 12:14 PM, Alexei Starovoitov wrote:
> On Fri, Sep 15, 2017 at 11:00:31AM -0600, Shuah Khan wrote:
>>>> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
>>>> the run_test>
>>> I'm not sure what was the motivation to exclude hotplug from default testing,
>>
>> These are considered a bit more disruptive and were excluded a while
>> back. These take cpus and memory on and off-line. Also require
>> root access. So even if they are included in the regular run, these
>> won't run.
>
> most of bpf tests require root access as well.
>
>> The first failure due to clang dependency is not a problem. The second one
>> in the case of "make kselftest" is the one that requires some work when bpf
>> make is run from the main Makefile. A lots of users run tests using the
>> kselftest target from the mail Makefile. hence I would like to get this
>> working, so it would be easier to run this test on test servers.
>
> 'make kselftest' doesn't work for me at all, since I suspect it
> assumes in-source build. I always use KBUILD_OUTPUT,
> since I build multiple archs with different configs out of the same
> source tree, so there is a bigger problem here.
Right. I think you probably use
make KBUILD_OUTPUT=dir -C tools/testing/selftests/bpf
>
> $ make kselftest
> make[1]: Entering directory `/data/users/ast/net-next/bld_x64'
> make: Entering an unknown directory
> make: *** tools/testing/selftests: No such file or directory. Stop.
> make: Leaving an unknown directory
> make[1]: *** [kselftest] Error 2
> make[1]: Leaving directory `/data/users/ast/net-next/bld_x64'
> make: *** [sub-make] Error 2
>
Right. There is patch set out to fix this problem which I sent out couple
of days ago. It is probably the case that bpf Makefile might need changes
to build when it is built from the sleftests/Makefile or the main Makefile.
thanks,
-- Shuah
^ permalink raw reply
* [PATCH] bgmac: Remove all offloading features, including GRO.
From: Rosen Penev @ 2017-09-15 22:22 UTC (permalink / raw)
To: netdev, zajec5, nbd; +Cc: Rosen Penev
On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
Currently:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52410
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 641 75:181:12:1:1:0:0:371
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52412
[ 5] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 629 51:194:13:1:0:1:0:369
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52414
[ 4] 0.00-10.02 sec 51.9 MBytes 43.4 Mbits/sec 695 126:203:1:0:0:0:2:363
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52416
[ 5] 0.00-10.01 sec 52.4 MBytes 43.9 Mbits/sec 626 57:186:10:0:0:0:0:373
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52420
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 605 36:179:16:1:0:1:0:372
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52422
After disabling everything - including GRO:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52440
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.01 sec 55.1 MBytes 46.2 Mbits/sec 672 180:82:0:0:1:0:0:409
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52442
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 636 117:96:0:0:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52444
[ 4] 0.00-10.01 sec 55.4 MBytes 46.4 Mbits/sec 675 172:92:0:0:1:0:0:410
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52446
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 633 119:90:0:1:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52448
[ 4] 0.00-10.01 sec 55.2 MBytes 46.3 Mbits/sec 688 157:123:0:0:2:0:0:406
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 48d672b204a4..c2db5673b073 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1534,7 +1534,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
goto err_dma_free;
}
- net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+ net_dev->features &= NETIF_F_GRO;
net_dev->hw_features = net_dev->features;
net_dev->vlan_features = net_dev->features;
--
2.13.5
^ permalink raw reply related
* Re: [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: David Miller @ 2017-09-15 22:02 UTC (permalink / raw)
To: ecree; +Cc: netdev, daniel
In-Reply-To: <8c3d5513-6171-3e68-56df-1efd0e87f071@solarflare.com>
From: Edward Cree <ecree@solarflare.com>
Date: Fri, 15 Sep 2017 14:37:38 +0100
> Neither ___bpf_prog_run nor the JITs accept it.
> Also adds a new test case.
>
> Fixes: 17a5267067f3 ("bpf: verifier (add verifier core)")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock
From: David Miller @ 2017-09-15 21:53 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, edumazet, jqiaoulk
In-Reply-To: <C414D433-28B3-4BA4-938B-B5C868918532@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 15 Sep 2017 14:28:40 -0700
> FYI, there is another patch needed to ensure consistency between
> ethtool reported stats and netdevice stats, will submit that after
> some more testing. Thanks!
Yes, I saw that, th anks.
^ permalink raw reply
* Re: [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: David Miller @ 2017-09-15 21:51 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman
In-Reply-To: <c5398945d4e80ed8b8497675dd0449763fec4eec.1505444568.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 15 Sep 2017 11:02:48 +0800
> sctp_diag would not actually dump out sk/asoc if inet_sctp_diag_fill
> returns err, in which case it shouldn't mark sk dumped by setting
> cb->args[3] as 1 in sctp_sock_dump().
>
> Otherwise, it could cause some asocs to have no parent's sk dumped
> in 'ss --sctp'.
>
> So this patch is to not set cb->args[3] when inet_sctp_diag_fill()
> returns err in sctp_sock_dump().
>
> Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied.
^ permalink raw reply
* Re: [Outreachy kernel] Re: [PATCH 2/2] Staging: irda: Remove parentheses on the right of assignment
From: Julia Lawall @ 2017-09-15 21:50 UTC (permalink / raw)
To: Joe Perches
Cc: Srishti Sharma, samuel, gregkh, netdev, devel, linux-kernel,
outreachy-kernel
In-Reply-To: <1505511108.27581.16.camel@perches.com>
On Fri, 15 Sep 2017, Joe Perches wrote:
> On Sat, 2017-09-16 at 02:36 +0530, Srishti Sharma wrote:
> > Parentheses are not needed on the right hand side of assignment
> > statement in most cases. Done using the following semantic
> > patch by coccinelle.
> []
> > @@
> > identifier E,F,G,f;
> > expression e,r;
> > @@
> >
> > (
> > E = (G == F);
> > >
> >
> > E = (e == r);
> > >
> >
> > E =
> > -(
> > ...
> > -)
> > ;
> > )
> []
> > diff --git a/drivers/staging/irda/drivers/mcs7780.c b/drivers/staging/irda/drivers/mcs7780.c
> []
> > @@ -605,7 +605,7 @@ static int mcs_speed_change(struct mcs_cb *mcs)
> > if (mcs->new_speed <= 115200) {
> > rval &= ~MCS_FIR;
> >
> > - rst = (mcs->speed > 115200);
> > + rst = mcs->speed > 115200;
> > if (rst)
> > mcs_set_reg(mcs, MCS_MINRXPW_REG, 0);
>
> Coccinelle is a good tool, but its output is limited to
> the correctness
> and completeness of its input script.
>
> Please look at the suggested modifications of the script
> and examine the code for other similar uses.
>
> The else if block immediately below this is:
>
> } else if (mcs->new_speed <= 1152000) {
> rval &= ~MCS_FIR;
>
> if ((rst = !(mcs->speed == 576000 || mcs->speed == 11520
> 00)))
> mcs_set_reg(mcs, MCS_MINRXPW_REG, 5);
>
> which should also be corrected by this patch.
You're concerned about the assignment in the if header? Because that was
in 1/2. One could also push the ! under the ||, but I'm not sure that
would be much of an improvement.
julia
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505511108.27581.16.camel%40perches.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply
* Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: David Miller @ 2017-09-15 21:48 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, pabeni
In-Reply-To: <9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 15 Sep 2017 11:02:21 +0800
> Commit 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the
> dump") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep
> with holding sock and sock lock.
>
> But Paolo noticed that endpoint could be destroyed in sctp_rcv without
> sock lock protection. It means the use-after-free issue still could be
> triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks
> !ep, although it's pretty hard to reproduce.
>
> I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close
> and sctp_sock_dump long time.
>
> This patch is to add another param cb_done to sctp_for_each_transport
> and dump ep->assocs with holding tsp after jumping out of transport's
> traversal in it to avoid this issue.
>
> It can also improve sctp diag dump to make it run faster, as no need
> to save sk into cb->args[5] and keep calling sctp_for_each_transport
> any more.
>
> This patch is also to use int * instead of int for the pos argument
> in sctp_for_each_transport, which could make postion increment only
> in sctp_for_each_transport and no need to keep changing cb->args[2]
> in sctp_sock_filter and sctp_sock_dump any more.
>
> Fixes: 86fdb3448cc1 ("sctp: ensure ep is not destroyed before doing the dump")
> Reported-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied.
^ 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