* 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
* Re: [PATCH net] netvsc: increase default receive buffer size
From: David Miller @ 2017-09-15 21:48 UTC (permalink / raw)
To: stephen; +Cc: netdev, haiyangz, sthemmin, devel
In-Reply-To: <20170914163107.8404-1-sthemmin@microsoft.com>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 14 Sep 2017 09:31:07 -0700
> The default receive buffer size was reduced by recent change
> to a value which was appropriate for 10G and Windows Server 2016.
> But the value is too small for full performance with 40G on Azure.
> Increase the default back to maximum supported by host.
>
> Fixes: 8b5327975ae1 ("netvsc: allow controlling send/recv buffer size")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] tcp: update skb->skb_mstamp more carefully
From: David Miller @ 2017-09-15 21:37 UTC (permalink / raw)
To: eric.dumazet
Cc: liujian56, edumazet, ycheng, hkchu, netdev, weiyongjun1,
wangkefeng.wang
In-Reply-To: <1505359839.15310.209.camel@edumazet-glaptop3.roam.corp.google.com>
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.
^ permalink raw reply
* Re: [PATCH 2/2] Staging: irda: Remove parentheses on the right of assignment
From: Joe Perches @ 2017-09-15 21:31 UTC (permalink / raw)
To: Srishti Sharma, samuel
Cc: gregkh, netdev, devel, linux-kernel, outreachy-kernel
In-Reply-To: <9380189edb2722544981c868699ade29ab5c297d.1505508423.git.srishtishar@gmail.com>
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.
^ permalink raw reply
* Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock
From: Florian Fainelli @ 2017-09-15 21:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev, edumazet, jqiaoulk
In-Reply-To: <20170915.142511.221689318302073484.davem@davemloft.net>
On September 15, 2017 2:25:11 PM PDT, David Miller <davem@davemloft.net> wrote:
>From: Florian Fainelli <f.fainelli@gmail.com>
>Date: Tue, 12 Sep 2017 13:14:26 -0700
>
>> We can enter a deadlock situation because there is no sufficient
>protection
>> when ndo_get_stats64() runs in process context to guard against RX or
>TX NAPI
>> contexts running in softirq, this can lead to the following lockdep
>splat and
>> actual deadlock was experienced as well with an iperf session in the
>background
>> and a while loop doing ifconfig + ethtool.
> ...
>> So just remove the u64_stats_update_begin()/end() pair in
>ndo_get_stats64()
>> since it does not appear to be useful for anything. No inconsistency
>was
>> observed with either ifconfig or ethtool, global TX counts equal the
>sum of
>> per-queue TX counts on a 32-bit architecture.
>>
>> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
>Applied.
FYI, there is another patch needed to ensure consistency between ethtool reported stats and netdevice stats, will submit that after some more testing. Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH] net: ipv4: fix l3slave check for index returned in IP_PKTINFO
From: David Miller @ 2017-09-15 21:28 UTC (permalink / raw)
To: dsahern; +Cc: netdev
In-Reply-To: <1505347897-27422-1-git-send-email-dsahern@gmail.com>
From: David Ahern <dsahern@gmail.com>
Date: Wed, 13 Sep 2017 17:11:37 -0700
> rt_iif is only set to the actual egress device for the output path. The
> recent change to consider the l3slave flag when returning IP_PKTINFO
> works for local traffic (the correct device index is returned), but it
> broke the more typical use case of packets received from a remote host
> always returning the VRF index rather than the original ingress device.
> Update the fixup to consider l3slave and rt_iif actually getting set.
>
> Fixes: 1dfa76390bf05 ("net: ipv4: add check for l3slave for index returned in IP_PKTINFO")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied, thanks David.
^ permalink raw reply
* Re: [PATCH v2] net: smsc911x: Quieten netif during suspend
From: David Miller @ 2017-09-15 21:26 UTC (permalink / raw)
To: geert+renesas
Cc: steve.glendinning, andrew, f.fainelli, netdev, linux-pm,
linux-renesas-soc, linux-kernel
In-Reply-To: <1505324525-9998-1-git-send-email-geert+renesas@glider.be>
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Wed, 13 Sep 2017 19:42:05 +0200
> If the network interface is kept running during suspend, the net core
> may call net_device_ops.ndo_start_xmit() while the Ethernet device is
> still suspended, which may lead to a system crash.
>
> E.g. on sh73a0/kzm9g and r8a73a4/ape6evm, the external Ethernet chip is
> driven by a PM controlled clock. If the Ethernet registers are accessed
> while the clock is not running, the system will crash with an imprecise
> external abort.
>
> As this is a race condition with a small time window, it is not so easy
> to trigger at will. Using pm_test may increase your chances:
>
> # echo 0 > /sys/module/printk/parameters/console_suspend
> # echo platform > /sys/power/pm_test
> # echo mem > /sys/power/state
>
> To fix this, make sure the network interface is quietened during
> suspend.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock
From: David Miller @ 2017-09-15 21:25 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, edumazet, jqiaoulk
In-Reply-To: <1505247266-42195-1-git-send-email-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 12 Sep 2017 13:14:26 -0700
> We can enter a deadlock situation because there is no sufficient protection
> when ndo_get_stats64() runs in process context to guard against RX or TX NAPI
> contexts running in softirq, this can lead to the following lockdep splat and
> actual deadlock was experienced as well with an iperf session in the background
> and a while loop doing ifconfig + ethtool.
...
> So just remove the u64_stats_update_begin()/end() pair in ndo_get_stats64()
> since it does not appear to be useful for anything. No inconsistency was
> observed with either ifconfig or ethtool, global TX counts equal the sum of
> per-queue TX counts on a 32-bit architecture.
>
> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: vrf: avoid gcc-4.6 warning
From: David Miller @ 2017-09-15 21:22 UTC (permalink / raw)
To: arnd; +Cc: dsa, shm, ja, mschiffer, weiwan, netdev, linux-kernel
In-Reply-To: <20170912201100.3680915-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 12 Sep 2017 22:10:53 +0200
> When building an allmodconfig kernel with gcc-4.6, we get a rather
> odd warning:
>
> drivers/net/vrf.c: In function ‘vrf_ip6_input_dst’:
> drivers/net/vrf.c:964:3: error: initialized field with side-effects overwritten [-Werror]
> drivers/net/vrf.c:964:3: error: (near initialization for ‘fl6’) [-Werror]
>
> I have no idea what this warning is even trying to say, but it does
> seem like a false positive. Reordering the initialization in to match
> the structure definition gets rid of the warning, and might also avoid
> whatever gcc thinks is wrong here.
>
> Fixes: 9ff74384600a ("net: vrf: Handle ipv6 multicast and link-local addresses")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
That's extremely weird, I can't explain it either.
Oh well, applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next v1] bridge: also trigger RTM_NEWLINK when interface is released from bridge
From: Vincent Bernat @ 2017-09-15 21:08 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, bridge, netdev
In-Reply-To: <20170915193808.22338-1-vincent@bernat.im>
❦ 15 septembre 2017 21:38 +0200, Vincent Bernat <vincent@bernat.im> :
> Currently, when an interface is released from a bridge, we get a
> RTM_DELLINK event through netlink:
>
> Deleted 2: dummy0: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 master bridge0 state UNKNOWN
> link/ether 6e:23:c2:54:3a:b3
It should be noted this only happens when using the ioctl API. When
using the netlink API, rtnetlink.c will send a RTM_NEWLINK because of
the modification.
--
When in doubt, tell the truth.
-- Mark Twain
^ 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