* Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Willem de Bruijn @ 2017-09-15 17:46 UTC (permalink / raw)
To: Cong Wang
Cc: nixiaoming, David Miller, Eric Dumazet, waltje, gw4pts,
Andrey Konovalov, Tobias Klauser, Philip Pettersson,
Alexander Potapenko, Network Development, LKML, dede.wu
In-Reply-To: <CAM_iQpWAeB+8bm9u+eHS3k_h5PSbnztXBvzTC=f0-tohdiSgzQ@mail.gmail.com>
On Fri, Sep 15, 2017 at 1:41 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming <nixiaoming@huawei.com> wrote:
>>> From: l00219569 <lisimin@huawei.com>
>>>
>>> If fanout_add is preempted after running po-> fanout = match
>>> and before running __fanout_link,
>>> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink
>>>
>>> so, we need add mutex_lock(&fanout_mutex) to __unregister_prot_hook
>>
>> The packet socket code has no shortage of locks, so there are many
>> ways to avoid the race condition between fanout_add and packet_set_ring.
>>
>> Another option would be to lock the socket when calling fanout_add:
>>
>> - return fanout_add(sk, val & 0xffff, val >> 16);
>> + lock_sock(sk);
>> + ret = fanout_add(sk, val & 0xffff, val >> 16);
>> + release_sock(sk);
>> + return ret;
>>
>
> I don't think this is an option, because __unregister_prot_hook()
> can be called without lock_sock(), for example in packet_notifier().
>
>
>> But, for consistency, and to be able to continue to make sense of the
>> locking policy, we should use the most appropriate lock. This
>> is po->bind_lock, as it ensures atomicity between testing whether
>> a protocol hook is active through po->running and the actual existence
>> of that hook on the protocol hook list.
>
> Yeah, register_prot_hook() and unregister_prot_hook() already assume
> bind_lock.
>
> [...]
>
>>> out:
>>> mutex_unlock(&fanout_mutex);
>>> + spin_unlock(&po->bind_lock);
>>
>> This function can call kzalloc with GFP_KERNEL, which may sleep. It is
>> not correct to sleep while holding a spinlock. Which is why I take the lock
>> later and test po->running again.
>
>
> Right, no need to mention the mutex_unlock() before the spin_unlock()
> is clearly wrong.
>
>
>>
>> I will clean up that patch and send it for review.
>
> How about the following patch?
>
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index c26172995511..f5c696a548ed 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id,
> u16 type_flags)
> match->prot_hook.dev == po->prot_hook.dev) {
> err = -ENOSPC;
> if (refcount_read(&match->sk_ref) < PACKET_FANOUT_MAX) {
> + spin_lock(&po->bind_lock);
> __dev_remove_pack(&po->prot_hook);
> - po->fanout = match;
> - refcount_set(&match->sk_ref,
> refcount_read(&match->sk_ref) + 1);
> - __fanout_link(sk, po);
> + if (po->running) {
> + refcount_set(&match->sk_ref,
> refcount_read(&match->sk_ref) + 1);
> + po->fanout = match;
> + __fanout_link(sk, po);
> + }
> + spin_unlock(&po->bind_lock);
> err = 0;
> }
> }
In case of failure we also need to unlink and free match. I
sent the following:
http://patchwork.ozlabs.org/patch/813945/
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 17:44 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <e07dabee-6c9c-7842-65b0-990a2ed1a2e9@osg.samsung.com>
On 09/15/2017 11:00 AM, Shuah Khan wrote:
> On 09/15/2017 10:02 AM, Alexei Starovoitov wrote:
>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>> Hi Alexei and Daniel,
>>>
>>> bpf test depends on clang and fails to compile when
>>>
>>> ------------------------------------------------------
>>> make -C tools/testing/selftests/bpf run_tests
>>>
>>>
>>> make: clang: Command not found
>>> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
>>> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
>>> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>>>
>>> With "make TARGETS=bpf kselftest" it fails earlier:
>>>
>>>
>>> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
>>> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
>>> Makefile:84: tools/build/Makefile.feature: No such file or directory
>>> Makefile:143: tools/build/Makefile.include: No such file or directory
>>> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
>>> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
>>> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
>>> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
>>> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
>>> Makefile:69: recipe for target 'all' failed
>>> make[1]: *** [all] Error 2
>>> Makefile:1190: recipe for target 'kselftest' failed
>>> make: *** [kselftest] Error 2
>>>
>>> --------------------------------------------------------------
>>>
>>> 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.
>> More details are in samples/bpf/README.rst
>> which was written long ago. Nowadays apt-get/yum will install clang
>> with bpf support builtin.
>
> Thanks for the clarification.
>
>>
>>> Would it make sense to create a special target for bpf test? We do have a few
>>> tests that do that now.
>>>
>>> TARGETS_HOTPLUG = cpu-hotplug
>>> TARGETS_HOTPLUG += memory-hotplug
>>>
>>> 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.
>
>> since I think it diminishes the value of selftests overall.
>
> I agree. We do have some timer tests that are destructive/stress that
> et run using a special target. It is the idea that if somebody wants
> to test all them, there is a way to do that.
>
> In any case, I didn't think bpf falls into this category of tests that
> belong in the destructive category. I am looking to understand the failures
> and your take on those.
>
>> Not running all tests all the time risks breaking them
> It is balance of providing a choice to users if they don't want to
> run destructive tests. For example, suspend test which requires root
> access. So the idea is for users to run these by choice as opposed
> to running them in the normal run and cause disruption.
>
>> selftest makefile refactoring broke selftests/bpf in the past,
>
> Yeah. We have had some fallout from the KBUILD_OUTPUT work that didn't
> take all the use-cases into account and tests that require custom
> builds such as the bpf tests. Using common build infrastructure doesn't
> work for all tests.
>
> Looks like there are other patches that went in later with lcap work.
>
>> so I strongly suggest to install clang and make sure the tests are passing
>> on the test servers
>
> You will have to request kernelci, stable, and It is a choice to be made by
> kernelci/zero-day folks.
>
> otherwise we'd need to move selftests/bpf out of
>> selftests to avoid further headaches for us when selftests infra keeps
>> changing.
>>
>
> Let's not go to extreme options. :) I am merely looking for more information
> and trying to understand the dependencies for this test.
>
> Let's look for a constructive option to fix the build failures I am seeing.
>
> The first failure due to clang dependency is not a problem.
Let me clarify that. People interested in running bpf test will have to
install clang. In that sense installing clang will solve that issue.
The hard dependency on clang does make it difficult for developers to
ensure they didn't break bpf when they make changes to the kselftest
common infrastructure.
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.
>
Even if this is fixed, 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. But that is your choice as to whether you want to increase the
scope of regression test coverage for bpf or not.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH] net/packet: fix race condition between fanout_add and __unregister_prot_hook
From: Cong Wang @ 2017-09-15 17:41 UTC (permalink / raw)
To: Willem de Bruijn
Cc: nixiaoming, David Miller, Eric Dumazet, waltje, gw4pts,
Andrey Konovalov, Tobias Klauser, Philip Pettersson,
Alexander Potapenko, Network Development, LKML, dede.wu
In-Reply-To: <CAF=yD-Jt5boxgQne4s1Fzy0y=Lq7RO4fjF6BfOCoh1bgaSECHQ@mail.gmail.com>
On Thu, Sep 14, 2017 at 7:35 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Thu, Sep 14, 2017 at 10:07 AM, nixiaoming <nixiaoming@huawei.com> wrote:
>> From: l00219569 <lisimin@huawei.com>
>>
>> If fanout_add is preempted after running po-> fanout = match
>> and before running __fanout_link,
>> it will cause BUG_ON when __unregister_prot_hook call __fanout_unlink
>>
>> so, we need add mutex_lock(&fanout_mutex) to __unregister_prot_hook
>
> The packet socket code has no shortage of locks, so there are many
> ways to avoid the race condition between fanout_add and packet_set_ring.
>
> Another option would be to lock the socket when calling fanout_add:
>
> - return fanout_add(sk, val & 0xffff, val >> 16);
> + lock_sock(sk);
> + ret = fanout_add(sk, val & 0xffff, val >> 16);
> + release_sock(sk);
> + return ret;
>
I don't think this is an option, because __unregister_prot_hook()
can be called without lock_sock(), for example in packet_notifier().
> But, for consistency, and to be able to continue to make sense of the
> locking policy, we should use the most appropriate lock. This
> is po->bind_lock, as it ensures atomicity between testing whether
> a protocol hook is active through po->running and the actual existence
> of that hook on the protocol hook list.
Yeah, register_prot_hook() and unregister_prot_hook() already assume
bind_lock.
[...]
>> out:
>> mutex_unlock(&fanout_mutex);
>> + spin_unlock(&po->bind_lock);
>
> This function can call kzalloc with GFP_KERNEL, which may sleep. It is
> not correct to sleep while holding a spinlock. Which is why I take the lock
> later and test po->running again.
Right, no need to mention the mutex_unlock() before the spin_unlock()
is clearly wrong.
>
> I will clean up that patch and send it for review.
How about the following patch?
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index c26172995511..f5c696a548ed 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1754,10 +1754,14 @@ static int fanout_add(struct sock *sk, u16 id,
u16 type_flags)
match->prot_hook.dev == po->prot_hook.dev) {
err = -ENOSPC;
if (refcount_read(&match->sk_ref) < PACKET_FANOUT_MAX) {
+ spin_lock(&po->bind_lock);
__dev_remove_pack(&po->prot_hook);
- po->fanout = match;
- refcount_set(&match->sk_ref,
refcount_read(&match->sk_ref) + 1);
- __fanout_link(sk, po);
+ if (po->running) {
+ refcount_set(&match->sk_ref,
refcount_read(&match->sk_ref) + 1);
+ po->fanout = match;
+ __fanout_link(sk, po);
+ }
+ spin_unlock(&po->bind_lock);
err = 0;
}
}
^ permalink raw reply related
* Re: scheduling while atomic from vmci_transport_recv_stream_cb in 3.16 kernels
From: Ben Hutchings @ 2017-09-15 17:12 UTC (permalink / raw)
To: Michal Hocko, Jorgen S. Hansen
Cc: Aditya Sarwade, Thomas Hellstrom, LKML, netdev@vger.kernel.org,
Masik Petr, Sasha Levin, Stable tree
In-Reply-To: <20170914085959.nkiefbzupwmknncy@dhcp22.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 509 bytes --]
On Thu, 2017-09-14 at 10:59 +0200, Michal Hocko wrote:
> On Wed 13-09-17 18:58:13, Jorgen S. Hansen wrote:
> [...]
> > The patch series look good to me.
>
> Thanks for double checking. Ben, could you merge this to 3.16 stable
> branch, please?
I have a long list of requests to work through, but I will get to this
eventually.
Ben.
--
Ben Hutchings
Kids! Bringing about Armageddon can be dangerous. Do not attempt it in
your own home. - Terry Pratchett and Neil Gaiman, `Good Omens'
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* (unknown),
From: noreply @ 2017-09-15 17:01 UTC (permalink / raw)
To: netdev
[-- Attachment #1: EMAIL_75480323541895_netdev.doc --]
[-- Type: application/msword, Size: 76645 bytes --]
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 17:00 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <20170915160253.q3x5j7hfkxxh2g6w@ast-mbp>
On 09/15/2017 10:02 AM, Alexei Starovoitov wrote:
> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>> Hi Alexei and Daniel,
>>
>> bpf test depends on clang and fails to compile when
>>
>> ------------------------------------------------------
>> make -C tools/testing/selftests/bpf run_tests
>>
>>
>> make: clang: Command not found
>> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
>> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
>> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>>
>> With "make TARGETS=bpf kselftest" it fails earlier:
>>
>>
>> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
>> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
>> Makefile:84: tools/build/Makefile.feature: No such file or directory
>> Makefile:143: tools/build/Makefile.include: No such file or directory
>> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
>> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
>> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
>> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
>> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
>> Makefile:69: recipe for target 'all' failed
>> make[1]: *** [all] Error 2
>> Makefile:1190: recipe for target 'kselftest' failed
>> make: *** [kselftest] Error 2
>>
>> --------------------------------------------------------------
>>
>> 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.
> More details are in samples/bpf/README.rst
> which was written long ago. Nowadays apt-get/yum will install clang
> with bpf support builtin.
Thanks for the clarification.
>
>> Would it make sense to create a special target for bpf test? We do have a few
>> tests that do that now.
>>
>> TARGETS_HOTPLUG = cpu-hotplug
>> TARGETS_HOTPLUG += memory-hotplug
>>
>> 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.
> since I think it diminishes the value of selftests overall.
I agree. We do have some timer tests that are destructive/stress that
et run using a special target. It is the idea that if somebody wants
to test all them, there is a way to do that.
In any case, I didn't think bpf falls into this category of tests that
belong in the destructive category. I am looking to understand the failures
and your take on those.
> Not running all tests all the time risks breaking them
It is balance of providing a choice to users if they don't want to
run destructive tests. For example, suspend test which requires root
access. So the idea is for users to run these by choice as opposed
to running them in the normal run and cause disruption.
> selftest makefile refactoring broke selftests/bpf in the past,
Yeah. We have had some fallout from the KBUILD_OUTPUT work that didn't
take all the use-cases into account and tests that require custom
builds such as the bpf tests. Using common build infrastructure doesn't
work for all tests.
Looks like there are other patches that went in later with lcap work.
> so I strongly suggest to install clang and make sure the tests are passing
> on the test servers
You will have to request kernelci, stable, and It is a choice to be made by
kernelci/zero-day folks.
otherwise we'd need to move selftests/bpf out of
> selftests to avoid further headaches for us when selftests infra keeps
> changing.
>
Let's not go to extreme options. :) I am merely looking for more information
and trying to understand the dependencies for this test.
Let's look for a constructive option to fix the build failures I am seeing.
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.
thanks,
-- Shuah
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Edward Cree @ 2017-09-15 16:58 UTC (permalink / raw)
To: Alexei Starovoitov, Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <20170915160253.q3x5j7hfkxxh2g6w@ast-mbp>
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.
-Ed
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Alexei Starovoitov @ 2017-09-15 16:02 UTC (permalink / raw)
To: Shuah Khan
Cc: Daniel Borkmann, Thomas Meyer, linux-kernel, linux-kselftest,
Shuah Khan, Networking
In-Reply-To: <295553a4-aadc-e5d1-229e-22d1966bc9f5@kernel.org>
On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
> Hi Alexei and Daniel,
>
> bpf test depends on clang and fails to compile when
>
> ------------------------------------------------------
> make -C tools/testing/selftests/bpf run_tests
>
>
> make: clang: Command not found
> Makefile:39: recipe for target '.linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o' failed
> make: *** [./linux-kselftest/tools/testing/selftests/bpf/test_pkt_access.o] Error 127
> make: Leaving directory '.linux-kselftest/tools/testing/selftests/bpf'
>
> With "make TARGETS=bpf kselftest" it fails earlier:
>
>
> make[3]: Entering directory './linux-kselftest/tools/lib/bpf'
> Makefile:40: tools/scripts/Makefile.arch: No such file or directory
> Makefile:84: tools/build/Makefile.feature: No such file or directory
> Makefile:143: tools/build/Makefile.include: No such file or directory
> make[3]: *** No rule to make target 'tools/build/Makefile.include'. Stop.
> make[3]: Leaving directory '.linux-kselftest/tools/lib/bpf'
> Makefile:34: recipe for target './linux-kselftest/tools/testing/selftests/bpf/libbpf.a' failed
> make[2]: *** [./linux-kselftest/tools/testing/selftests/bpf/libbpf.a] Error 2
> make[2]: Leaving directory './linux-kselftest/tools/testing/selftests/bpf'
> Makefile:69: recipe for target 'all' failed
> make[1]: *** [all] Error 2
> Makefile:1190: recipe for target 'kselftest' failed
> make: *** [kselftest] Error 2
>
> --------------------------------------------------------------
>
> 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.
More details are in samples/bpf/README.rst
which was written long ago. Nowadays apt-get/yum will install clang
with bpf support builtin.
> Would it make sense to create a special target for bpf test? We do have a few
> tests that do that now.
>
> TARGETS_HOTPLUG = cpu-hotplug
> TARGETS_HOTPLUG += memory-hotplug
>
> I could add a special target for bpf TARGET_BPF perhaps and exclude it from
> the run_tests.
I'm not sure what was the motivation to exclude hotplug from default testing,
since I think it diminishes the value of selftests overall.
Not running all tests all the time risks breaking them.
selftest makefile refactoring broke selftests/bpf in the past,
so I strongly suggest to install clang and make sure the tests are passing
on the test servers otherwise we'd need to move selftests/bpf out of
selftests to avoid further headaches for us when selftests infra keeps
changing.
^ permalink raw reply
* Re: [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: Alexei Starovoitov @ 2017-09-15 15:50 UTC (permalink / raw)
To: Edward Cree; +Cc: David Miller, netdev, Daniel Borkmann
In-Reply-To: <8c3d5513-6171-3e68-56df-1efd0e87f071@solarflare.com>
On Fri, Sep 15, 2017 at 02:37:38PM +0100, Edward Cree wrote:
> 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>
Good catch. Thanks!
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
From: Larry Finger @ 2017-09-15 15:19 UTC (permalink / raw)
To: Zwindl
Cc: linux-wireless@vger.kernel.org, chaoming_li@realsil.com.cn,
kvalo@codeaurora.org, pkshih@realtek.com, johannes.berg@intel.com,
gregkh@linuxfoundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <jB0WJNG17mSfKo9m7T26r-b2Gr7mDL4lJ-1cWxeCJ6UCsA4TsxXz4znCLhYY5EASPQciwp1rf_xtwvDR-xzwD-OTApTUFoaVmIvahleDcJk=@protonmail.com>
On 09/15/2017 05:10 AM, Zwindl wrote:
>
>> -------- Original Message --------
>> Subject: Re: RTL8192EE PCIe Wireless Network Adapter crashed with linux-4.13
>> Local Time: 14 September 2017 6:05 PM
>> UTC Time: 14 September 2017 18:05
>> From: Larry.Finger@lwfinger.net
>> To: Zwindl <zwindl@protonmail.com>, linux-wireless@vger.kernel.org
>> <linux-wireless@vger.kernel.org>
>> chaoming_li@realsil.com.cn <chaoming_li@realsil.com.cn>, kvalo@codeaurora.org
>> <kvalo@codeaurora.org>, pkshih@realtek.com <pkshih@realtek.com>,
>> johannes.berg@intel.com <johannes.berg@intel.com>, gregkh@linuxfoundation.org
>> <gregkh@linuxfoundation.org>, netdev@vger.kernel.org <netdev@vger.kernel.org>,
>> linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
>>
>> On 09/14/2017 08:30 AM, Zwindl wrote:
>> > Dear developers:
>> > I"m using Arch Linux with testing enabled, the current kernel version and
>> > details are
>> > `Linux zwindl 4.13.2-1-ARCH #1 SMP PREEMPT Thu Sep 14 02:57:34 UTC 2017 x86_64
>> > GNU/Linux`.
>> > The wireless card can"t work properly from the kernel 4.13. Here"s the log(in
>> > attachment) when NetworkManager trying to connect my wifi which is named as
>> > "TP", my mac addr hided as xx:xx:xx:xx:xx.
>> > What should I provide to help to debug?
>> > ZWindL.
>>
>> The BUG-ON arises in __intel_map_single() due to dir (for direction of DMA)
>> equal to DMA_NONE (3). When rtl8192ee calls pci_map_single(), it uses
>> PCI_DMA_TODEVICE (1). I followed the calling sequence through the entire chain,
>> and none of the routines made any changes to "dir", other that changing the type
>> from int to enum dma_data_direction. That would not have changed a 1 to a 3.
>>
>> I built a 4.13.2 system. The problem does not happen here. At this point, the
>> system has been up for about two hours. I did discover a small memory leak
>> associated with firmware loading, but that should not have caused the problem.
>> Nonetheless, I will be sending a patch to fix that problem.
>>
>> I will continue testing, although I doubt that the problem will happen here.
>>
>> How long had your system been up when the problem occurred? Your dmesg fragment
>> did not show any times. What kernels have you tried besides 4.13.2?
>>
>> Larry
> Oh, sorry, the original log is from `journalctl`.
> Here's the `dmesg` prints(error.txt). I can't determine which part is related,
> so I paste all of it. I've tried 4.12.X(no issue), 4.13.1(issue), 4.13.2(issue).
> ZWindL
The output of dmesg is a lot more instructive than that of journalctl. I now
know exactly the location that triggered the WARNING. I still do not understand
it. In fact, it is likely a regression in kernel 4.13 that does not affect my
Toshiba laptop, nor a Lenovo machine I have, but does affect your Lenovo laptop.
Is it possible for you to install the mainline source from vger.kernel.org using
git and bisect the issue? It will take quite a bit of time, but it is likely the
only way to find the offending change. If you are willing to try this, I will
send you reasonably complete instructions.
By the way, it is usually better to load the dmesg output into a pastebin site
and post the link. Sending the entire file to a list makes a lot of people
receive a lot of data for which they have no interest.
Larry
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-15 15:19 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915142617.GA2060@nanopsycho.orion>
> > Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
> >-----------------------------------------------------------------------------
> > 00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
> > 01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
> > 02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
> > 04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
> > 05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
> > 06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
> > 07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
> > 08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
> > 09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
> > 0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
> > 0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 0f: 9100 dada dada dada dada dada dada 0000 00000
> > 10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
> > 14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
> > 17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
> > 19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
> > 1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
> > 1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> > 1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> >
>
> Is this a reg dump per-port?
No. Look at the global0 and global1 columns. These are not port
registers, but switch global registers. The other columns are per
port. We could have a two column table per port for these
registers. However for debugging, it is useful to see the port
registers side-by-side. The difference between ports often gives you
clues as to what is wrong.
The global registers however are scoped to a switch, not a port.
> you can have reg array for each. How the values can change? Is this
> change result of driver<->hw communication? If yes, you might consider
> using devlink hwmsg trace to expose the communication to userspace.
The reason a value changes varies per bit. Some are status, set by the
switch. Some are configuration, set by the driver. They are all mixed
up together. And some registers are actually just multiplexors onto
other tables of registers. You set an opcode in the lower nibble,
address in the middle, set the top bit and busy loop waiting for it to
clear. You can then read a result out of the register, or a near by
register. So the register dump gives you an idea what the last access
to such a table was.
> I would rather focus on what exactly you need to expose to userspace,
> then we can figure out how to do it. Generic multipurpose arrays should
> be considered as last-resort solution in my opinion.
So go look at Vivien's patches. That is what we want to expose. For a
start. Viviens patches are at the DSA level. Once we get something
accepted at that level, i can imaging drivers want to augment it with
driver specific tables. But we can handle that later.
Andrew
^ permalink raw reply
* Re: [PATCH net] l2tp: fix race condition in l2tp_tunnel_delete
From: Sabrina Dubroca @ 2017-09-15 14:55 UTC (permalink / raw)
To: Tom Parkin; +Cc: netdev, Guillaume Nault, Xin Long
In-Reply-To: <20170915094259.GA3209@jackdaw>
2017-09-15, 10:42:59 +0100, Tom Parkin wrote:
> On Fri, Sep 15, 2017 at 11:08:07AM +0200, Sabrina Dubroca wrote:
> > The tunnel is currently removed from the list during destruction. This
> > can lead to a double-free of the struct sock if we try to delete the tunnel
> > twice fast enough.
> >
> > The first delete operation does a lookup (l2tp_tunnel_get), finds the
> > tunnel, calls l2tp_tunnel_delete, which queues it for deletion by
> > l2tp_tunnel_del_work.
> >
> > The second delete operation also finds the tunnel and calls
> > l2tp_tunnel_delete. If the workqueue has already fired and started
> > running l2tp_tunnel_del_work, then l2tp_tunnel_delete will queue the
> > same tunnel a second time, and try to free the socket again.
> >
> > Add a dead flag and remove tunnel from its list earlier. Then we can
> > remove the check of queue_work's result that was meant to prevent that
> > race but doesn't.
>
> How do we avoid leaving stale information on the tunnel list for
> use-cases which don't delete tunnels using netlink? For example the
> L2TPv2 ppp/socket API depends on sk_destruct to clean up the kernel
> context on socket destruction. Similarly, userspace may just close
> the tunnel socket without first making netlink calls to delete the
> tunnel.
>
> By moving the tunnel list removal from l2tp_tunnel_destruct to
> l2tp_tunnel_delete I can't see how codepaths which don't involve
> l2tp_tunnel_delete don't end up with a corrupted tunnel list.
Ok, thanks for pointing that out. We could go with just the ->dead
flag then. I'm not sure whether we need to set it in
l2tp_tunnel_destruct as well.
-------- 8< --------
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ee485df73ccd..e74596418169 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1685,14 +1685,12 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
/* This function is used by the netlink TUNNEL_DELETE command.
*/
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
{
- l2tp_tunnel_inc_refcount(tunnel);
- if (false == queue_work(l2tp_wq, &tunnel->del_work)) {
- l2tp_tunnel_dec_refcount(tunnel);
- return 1;
+ if (!test_and_set_bit(1, &tunnel->dead)) {
+ l2tp_tunnel_inc_refcount(tunnel);
+ queue_work(l2tp_wq, &tunnel->del_work);
}
- return 0;
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a305e0c5925a..deda869504d0 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,6 +160,9 @@ struct l2tp_tunnel_cfg {
struct l2tp_tunnel {
int magic; /* Should be L2TP_TUNNEL_MAGIC */
+
+ unsigned long dead;
+
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
bool acpt_newsess; /* Indicates whether this
@@ -254,7 +257,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
struct l2tp_tunnel **tunnelp);
void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
-int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
+void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
--
Sabrina
^ permalink raw reply related
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Jiri Pirko @ 2017-09-15 14:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915140839.GD3084@lunn.ch>
Fri, Sep 15, 2017 at 04:08:39PM CEST, andrew@lunn.ch wrote:
>> Could you put together your requirements so we can work it out to extend
>> devlink to support them?
>
>As i've said multiple times, generic two dimensional tables. Examples
>could look like:
>
>Stats cpu lan0 lan1 lan2 lan3 lan4 dsa
>---------------------------------------------------------------------
>tx_packets: 2 0 0 0 0 0 0
>tx_bytes: 1180 6666 0 0 0 0 0
>rx_packets: 0 0 0 0 0 0 0
>rx_bytes: 0 1180 0 0 0 0 0
>in_good_octets: 6666 1188 0 0 0 0 0
>in_bad_octets: 0 0 0 0 0 0 0
>in_unicast: 0 0 0 0 0 0 0
>in_broadcasts: 0 0 0 0 0 0 0
>in_multicasts: 89 0 0 0 0 0 0
>in_pause: 0 0 0 0 0 0 0
>in_undersize: 0 0 0 0 0 0 0
>in_fragments: 0 0 0 0 0 0 0
>in_oversize: 0 0 0 0 0 0 0
>in_jabber: 0 0 0 0 0 0 0
>in_rx_error: 0 0 0 0 0 0 0
>in_fcs_error: 0 0 0 0 0 0 0
>out_octets: 1188 6666 0 0 0 0 0
>out_unicast: 0 0 0 0 0 0 0
>out_broadcasts: 2 2 0 0 0 0 0
>out_multicasts: 0 89 0 0 0 0 0
>out_pause: 0 0 0 0 0 0 0
>excessive: 0 0 0 0 0 0 0
>collisions: 0 0 0 0 0 0 0
>deferred: 0 0 0 0 0 0 0
>single: 0 0 0 0 0 0 0
>multiple: 0 0 0 0 0 0 0
>out_fcs_error: 0 0 0 0 0 0 0
>late: 0 0 0 0 0 0 0
>hist_64bytes: 0 0 0 0 0 0 0
>hist_65_127bytes: 0 0 0 0 0 0 0
>hist_128_255bytes: 0 0 0 0 0 0 0
>hist_256_511bytes: 0 0 0 0 0 0 0
>hist_512_1023bytes: 0 0 0 0 0 0 0
>hist_1024_max_bytes: 0 0 0 0 0 0 0
>sw_in_discards: 0 0 0 0 0 0 0
>sw_in_filtered: 0 0 0 0 0 0 0
>sw_out_filtered: 89 89 0 0 0 0 0
I believe we discussed this already. You can have devlink_port instance
for each port, then you just have stats-per-devlink_port. Looks quite
easy to implement actually.
Would be mistake to have this as just 2 dim array.
>
>
> Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
>-----------------------------------------------------------------------------
> 00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
> 01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
> 02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
> 04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
> 05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
> 06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
> 07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
> 08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
> 09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
> 0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
> 0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 0f: 9100 dada dada dada dada dada dada 0000 00000
> 10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
> 14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
> 17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
> 19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
> 1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
> 1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
> 1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
>
Is this a reg dump per-port? Also, with per-port devlink_port instance,
you can have reg array for each. How the values can change? Is this
change result of driver<->hw communication? If yes, you might consider
using devlink hwmsg trace to expose the communication to userspace.
>So a table would have an optional header row. Then a number of data
>rows. The number of columns is the same for each row. The number of
>columns is determined at run time, but is known at the beginning of
>enumerating the table. The number of data rows is not known until the
>last one is enumerated.
I would rather focus on what exactly you need to expose to userspace,
then we can figure out how to do it. Generic multipurpose arrays should
be considered as last-resort solution in my opinion.
>
>Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
>ifindex, MAC address, IP address, devlink port, ....
>
>Each cell in a row can have a different type. The types of the header
>row cells can be different to the types of the data cells. All data
>rows have the same type information. So you can enumerate the types
>once, and use them for the whole table.
>
>The userspace tool should make its best effort to print the table,
>using the type info. It might need hits from the command line, like -x
>to print in hex not decimal. The second example shows this. Registers
>make more sense in hex, where as statistics make more sense in
>decimal. But let the user choose, so keeping the kAPI simple. This is
>intended as a debug tool. The output does not need to be highly
>polished. But it needs to be a lot more readable than the current
>dpipe output which prints a table as a list, not a table.
>
>The tables can be per port, or per switch. Remember that DSA allows a
>cluster of switches within one DSA instance.
>
>Vivien, Florian, did i miss anything?
>
> Andrew
^ permalink raw reply
* Re: [PATCH] net: phy: Fix mask value write on gmii2rgmii converter speed register.
From: Andrew Lunn @ 2017-09-15 14:10 UTC (permalink / raw)
To: Michal Simek
Cc: Fahad Kunnathadi, f.fainelli, netdev, linux-kernel,
soren.brinkmann, linux-arm-kernel
In-Reply-To: <5f4bd003-d251-5a14-497a-9cc72dd0f160@xilinx.com>
> Can you please point me to that email?
I assume you can search the email lists just as well as i can.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 01/10] net: dsa: add debugfs interface
From: Andrew Lunn @ 2017-09-15 14:08 UTC (permalink / raw)
To: Jiri Pirko
Cc: Alexander Duyck, Maxim Uvarov, Vivien Didelot, netdev, kernel,
Florian Fainelli, Egil Hjelmeland, John Crispin, Woojung Huh,
Sean Wang, Nikita Yushchenko, Chris Healy
In-Reply-To: <20170915055107.GA1927@nanopsycho.orion>
> Could you put together your requirements so we can work it out to extend
> devlink to support them?
As i've said multiple times, generic two dimensional tables. Examples
could look like:
Stats cpu lan0 lan1 lan2 lan3 lan4 dsa
---------------------------------------------------------------------
tx_packets: 2 0 0 0 0 0 0
tx_bytes: 1180 6666 0 0 0 0 0
rx_packets: 0 0 0 0 0 0 0
rx_bytes: 0 1180 0 0 0 0 0
in_good_octets: 6666 1188 0 0 0 0 0
in_bad_octets: 0 0 0 0 0 0 0
in_unicast: 0 0 0 0 0 0 0
in_broadcasts: 0 0 0 0 0 0 0
in_multicasts: 89 0 0 0 0 0 0
in_pause: 0 0 0 0 0 0 0
in_undersize: 0 0 0 0 0 0 0
in_fragments: 0 0 0 0 0 0 0
in_oversize: 0 0 0 0 0 0 0
in_jabber: 0 0 0 0 0 0 0
in_rx_error: 0 0 0 0 0 0 0
in_fcs_error: 0 0 0 0 0 0 0
out_octets: 1188 6666 0 0 0 0 0
out_unicast: 0 0 0 0 0 0 0
out_broadcasts: 2 2 0 0 0 0 0
out_multicasts: 0 89 0 0 0 0 0
out_pause: 0 0 0 0 0 0 0
excessive: 0 0 0 0 0 0 0
collisions: 0 0 0 0 0 0 0
deferred: 0 0 0 0 0 0 0
single: 0 0 0 0 0 0 0
multiple: 0 0 0 0 0 0 0
out_fcs_error: 0 0 0 0 0 0 0
late: 0 0 0 0 0 0 0
hist_64bytes: 0 0 0 0 0 0 0
hist_65_127bytes: 0 0 0 0 0 0 0
hist_128_255bytes: 0 0 0 0 0 0 0
hist_256_511bytes: 0 0 0 0 0 0 0
hist_512_1023bytes: 0 0 0 0 0 0 0
hist_1024_max_bytes: 0 0 0 0 0 0 0
sw_in_discards: 0 0 0 0 0 0 0
sw_in_filtered: 0 0 0 0 0 0 0
sw_out_filtered: 89 89 0 0 0 0 0
Reg cpu lan0 lan1 lan2 lan3 lan4 lan5 global0 global1
-----------------------------------------------------------------------------
00: 4e07 4d04 4d04 4d04 4d04 4d04 4d04 0000 00000
01: 403e 003d 003d 003d 003d 003d 003d 0000 00000
02: 0000 0000 0000 0000 0000 0000 0000 0000 00000
03: 3521 3521 3521 3521 3521 3521 3521 0000 00000
04: 0533 373f 373f 373f 373f 373f 373f 0000 00000
05: 8000 0000 0000 0000 0000 0000 0000 0000 00000
06: 005f 003f 003f 003f 003f 003f 003f 0000 00000
07: 002a 002a 002a 002a 002a 002a 002a 0000 00000
08: 2080 2080 2080 2080 2080 2080 2080 0000 00000
09: 0001 0001 0001 0001 0001 0001 0001 0000 00000
0a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0b: 0020 0000 0000 0000 0000 0000 0000 0000 00000
0c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
0f: 9100 dada dada dada dada dada dada 0000 00000
10: 0000 0000 0000 0000 0000 0000 0000 0000 00000
11: 0000 0000 0000 0000 0000 0000 0000 0000 00000
12: 0000 0000 0000 0000 0000 0000 0000 0000 00000
13: 0000 00d8 00d8 00d8 00d8 00d8 00d8 0000 00000
14: 0000 0000 0000 0000 0000 0000 0000 0000 00000
15: 0000 0000 0000 0000 0000 0000 0000 0000 00000
16: 0022 0000 0000 0000 0000 0000 0000 0000 00000
17: 0000 0000 0000 0000 0000 0000 0000 0000 00000
18: 3210 2210 2210 2210 2210 2210 2210 0000 00000
19: 7654 7654 7654 7654 7654 7654 7654 0000 00000
1a: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1b: 8000 8000 8000 8000 8000 8000 8000 0000 00000
1c: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1d: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1e: 0000 0000 0000 0000 0000 0000 0000 0000 00000
1f: 0000 0000 0000 0000 0000 0000 0000 0000 00000
So a table would have an optional header row. Then a number of data
rows. The number of columns is the same for each row. The number of
columns is determined at run time, but is known at the beginning of
enumerating the table. The number of data rows is not known until the
last one is enumerated.
Each cell in the row is typed. Can be a string, bool, u8, u16, u32, u64,
ifindex, MAC address, IP address, devlink port, ....
Each cell in a row can have a different type. The types of the header
row cells can be different to the types of the data cells. All data
rows have the same type information. So you can enumerate the types
once, and use them for the whole table.
The userspace tool should make its best effort to print the table,
using the type info. It might need hits from the command line, like -x
to print in hex not decimal. The second example shows this. Registers
make more sense in hex, where as statistics make more sense in
decimal. But let the user choose, so keeping the kAPI simple. This is
intended as a debug tool. The output does not need to be highly
polished. But it needs to be a lot more readable than the current
dpipe output which prints a table as a list, not a table.
The tables can be per port, or per switch. Remember that DSA allows a
cluster of switches within one DSA instance.
Vivien, Florian, did i miss anything?
Andrew
^ permalink raw reply
* Re: [PATCH net] packet: hold bind lock when rebinding to fanout hook
From: Willem de Bruijn @ 2017-09-15 14:07 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, David Miller, nixiaoming
In-Reply-To: <20170914211441.67326-1-willemb@google.com>
On Thu, Sep 14, 2017 at 5:14 PM, Willem de Bruijn <willemb@google.com> wrote:
> Packet socket bind operations must hold the po->bind_lock. This keeps
> po->running consistent with whether the socket is actually on a ptype
> list to receive packets.
>
> fanout_add unbinds a socket and its packet_rcv/tpacket_rcv call, then
> binds the fanout object to receive through packet_rcv_fanout.
>
> Make it hold the po->bind_lock when testing po->running and rebinding.
> Else, it can race with other rebind operations, such as that in
> packet_set_ring from packet_rcv to tpacket_rcv. Concurrent updates
> can result in a socket being added to a fanout group twice, causing
> use-after-free KASAN bug reports, among others.
>
> Reported independently by both trinity and syzkaller.
> Verified that the syzkaller reproducer passes after this patch.
>
I forgot to add the Fixes tag, sorry.
Fixes: dc99f600698d ("packet: Add fanout support.")
> Reported-by: nixioaming <nixiaoming@huawei.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [REGRESSION] Warning in tcp_fastretrans_alert() of net/ipv4/tcp_input.c
From: Neal Cardwell @ 2017-09-15 14:03 UTC (permalink / raw)
To: Oleksandr Natalenko
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Netdev,
Yuchung Cheng
In-Reply-To: <1760119.uO98D2ft6f@natalenko.name>
On Fri, Sep 15, 2017 at 1:03 AM, Oleksandr Natalenko
<oleksandr@natalenko.name> wrote:
> Hi.
>
> I've applied your test patch but it doesn't fix the issue for me since the
> warning is still there.
>
> Were you able to reproduce it?
Hi,
Thanks for testing that. That is a very useful data point.
I was able to cook up a packetdrill test that could put the connection
in CA_Disorder with retransmitted packets out, but not in CA_Open. So
we do not yet have a test case to reproduce this.
We do not see this warning on our fleet at Google. One significant
difference I see between our environment and yours is that it seems
you run with FACK enabled:
net.ipv4.tcp_fack = 1
Note that FACK was disabled by default (since it was replaced by RACK)
between kernel v4.10 and v4.11. And this is exactly the time when this
bug started manifesting itself for you and some others, but not our
fleet. So my new working hypothesis would be that this warning is due
to a behavior that only shows up in kernels >=4.11 when FACK is
enabled.
Would you be able to disable FACK ("sysctl net.ipv4.tcp_fack=0" at
boot, or net.ipv4.tcp_fack=0 in /etc/sysctl.conf, or equivalent),
reboot, and test the kernel for a few days to see if the warning still
pops up?
thanks,
neal
[ps: apologies for the previous, mis-formatted post...]
^ permalink raw reply
* [PATCH net] bpf/verifier: reject BPF_ALU64|BPF_END
From: Edward Cree @ 2017-09-15 13:37 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Daniel Borkmann
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>
---
kernel/bpf/verifier.c | 3 ++-
tools/testing/selftests/bpf/test_verifier.c | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 477b693..799b245 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -2292,7 +2292,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
}
} else {
if (insn->src_reg != BPF_REG_0 || insn->off != 0 ||
- (insn->imm != 16 && insn->imm != 32 && insn->imm != 64)) {
+ (insn->imm != 16 && insn->imm != 32 && insn->imm != 64) ||
+ BPF_CLASS(insn->code) == BPF_ALU64) {
verbose("BPF_END uses reserved fields\n");
return -EINVAL;
}
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 8eb0995..26f3250 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -6629,6 +6629,22 @@ static struct bpf_test tests[] = {
.result = REJECT,
.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,
},
+ {
+ "invalid 64-bit BPF_END",
+ .insns = {
+ BPF_MOV32_IMM(BPF_REG_0, 0),
+ {
+ .code = BPF_ALU64 | BPF_END | BPF_TO_LE,
+ .dst_reg = BPF_REG_0,
+ .src_reg = 0,
+ .off = 0,
+ .imm = 32,
+ },
+ BPF_EXIT_INSN(),
+ },
+ .errstr = "BPF_END uses reserved fields",
+ .result = REJECT,
+ },
};
static int probe_filter_length(const struct bpf_insn *fp)
^ permalink raw reply related
* [patch net] mlxsw: spectrum_router: Only handle IPv4 and IPv6 events
From: Jiri Pirko @ 2017-09-15 13:31 UTC (permalink / raw)
To: netdev; +Cc: davem, idosch, flokli, andreas, mlxsw
From: Ido Schimmel <idosch@mellanox.com>
The driver doesn't support events from address families other than IPv4
and IPv6, so ignore them. Otherwise, we risk queueing a work item before
it's initialized.
This can happen in case a VRF is configured when MROUTE_MULTIPLE_TABLES
is enabled, as the VRF driver will try to add an l3mdev rule for the
IPMR family.
Fixes: 65e65ec137f4 ("mlxsw: spectrum_router: Don't ignore IPv6 notifications")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Andreas Rammhold <andreas@rammhold.de>
Reported-by: Florian Klink <flokli@flokli.de>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index f0fb898..2cfb3f5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -4868,7 +4868,8 @@ static int mlxsw_sp_router_fib_event(struct notifier_block *nb,
struct fib_notifier_info *info = ptr;
struct mlxsw_sp_router *router;
- if (!net_eq(info->net, &init_net))
+ if (!net_eq(info->net, &init_net) ||
+ (info->family != AF_INET && info->family != AF_INET6))
return NOTIFY_DONE;
fib_work = kzalloc(sizeof(*fib_work), GFP_ATOMIC);
--
2.9.3
^ permalink raw reply related
* Re: feature request for kernel module 8021q
From: Pierre Colombier @ 2017-09-15 13:02 UTC (permalink / raw)
To: Florian Fainelli, netdev
In-Reply-To: <A4D84B7F-5D4A-44C8-875C-C113D7C5C688@gmail.com>
>> One interesting feature would be to have a special vlan number
>>
>> (let's say 0 or -1 or 4097 ) designed so that it gets all the untagged
>> trafic and only the untagged trafic.
> 4097 is not a valid number nor is -1 since that closely follows what the VLAN ID extracted from the tag would be. VLAN 0 is more or less the untagged VLAN.
I know -1 or 4097 are invalid numbers
This is precicely why i suggested them as "special"
I wasn't aware of the existing Vlan 0 feature
I just tried again and it seems to be exactly what I was requesting.
What do you mean by "more or less" ?
^ permalink raw reply
* Re: [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: Neil Horman @ 2017-09-15 12:59 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <c5398945d4e80ed8b8497675dd0449763fec4eec.1505444568.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:48AM +0800, Xin Long wrote:
> 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>
> ---
> net/sctp/sctp_diag.c | 1 -
> 1 file changed, 1 deletion(-)
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: Neil Horman @ 2017-09-15 12:57 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner, pabeni
In-Reply-To: <9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:
> 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>
> ---
> include/net/sctp/sctp.h | 3 ++-
> net/sctp/sctp_diag.c | 32 +++++++++-----------------------
> net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------
> 3 files changed, 36 insertions(+), 39 deletions(-)
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net] sctp: do not mark sk dumped when inet_sctp_diag_fill returns err
From: Marcelo Ricardo Leitner @ 2017-09-15 12:55 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <c5398945d4e80ed8b8497675dd0449763fec4eec.1505444568.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:48AM +0800, Xin Long wrote:
> 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>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/sctp_diag.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> index 7008a99..22ed01a 100644
> --- a/net/sctp/sctp_diag.c
> +++ b/net/sctp/sctp_diag.c
> @@ -309,7 +309,6 @@ static int sctp_sock_dump(struct sctp_transport *tsp, void *p)
> cb->nlh->nlmsg_seq,
> NLM_F_MULTI, cb->nlh,
> commp->net_admin) < 0) {
> - cb->args[3] = 1;
> err = 1;
> goto release;
> }
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump
From: Marcelo Ricardo Leitner @ 2017-09-15 12:55 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, pabeni
In-Reply-To: <9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>
On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:
> 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>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> include/net/sctp/sctp.h | 3 ++-
> net/sctp/sctp_diag.c | 32 +++++++++-----------------------
> net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------
> 3 files changed, 36 insertions(+), 39 deletions(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 06b4f51..d7d8cba 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -127,7 +127,8 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),
> const union sctp_addr *laddr,
> const union sctp_addr *paddr, void *p);
> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
> - struct net *net, int pos, void *p);
> + int (*cb_done)(struct sctp_transport *, void *),
> + struct net *net, int *pos, void *p);
> int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void *p);
> int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,
> struct sctp_info *info);
> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> index e99518e..7008a99 100644
> --- a/net/sctp/sctp_diag.c
> +++ b/net/sctp/sctp_diag.c
> @@ -279,9 +279,11 @@ static int sctp_tsp_dump_one(struct sctp_transport *tsp, void *p)
> return err;
> }
>
> -static int sctp_sock_dump(struct sock *sk, void *p)
> +static int sctp_sock_dump(struct sctp_transport *tsp, void *p)
> {
> + struct sctp_endpoint *ep = tsp->asoc->ep;
> struct sctp_comm_param *commp = p;
> + struct sock *sk = ep->base.sk;
> struct sk_buff *skb = commp->skb;
> struct netlink_callback *cb = commp->cb;
> const struct inet_diag_req_v2 *r = commp->r;
> @@ -289,9 +291,7 @@ static int sctp_sock_dump(struct sock *sk, void *p)
> int err = 0;
>
> lock_sock(sk);
> - if (!sctp_sk(sk)->ep)
> - goto release;
> - list_for_each_entry(assoc, &sctp_sk(sk)->ep->asocs, asocs) {
> + list_for_each_entry(assoc, &ep->asocs, asocs) {
> if (cb->args[4] < cb->args[1])
> goto next;
>
> @@ -327,40 +327,30 @@ static int sctp_sock_dump(struct sock *sk, void *p)
> cb->args[4]++;
> }
> cb->args[1] = 0;
> - cb->args[2]++;
> cb->args[3] = 0;
> cb->args[4] = 0;
> release:
> release_sock(sk);
> - sock_put(sk);
> return err;
> }
>
> -static int sctp_get_sock(struct sctp_transport *tsp, void *p)
> +static int sctp_sock_filter(struct sctp_transport *tsp, void *p)
> {
> struct sctp_endpoint *ep = tsp->asoc->ep;
> struct sctp_comm_param *commp = p;
> struct sock *sk = ep->base.sk;
> - struct netlink_callback *cb = commp->cb;
> const struct inet_diag_req_v2 *r = commp->r;
> struct sctp_association *assoc =
> list_entry(ep->asocs.next, struct sctp_association, asocs);
>
> /* find the ep only once through the transports by this condition */
> if (tsp->asoc != assoc)
> - goto out;
> + return 0;
>
> if (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family)
> - goto out;
> -
> - sock_hold(sk);
> - cb->args[5] = (long)sk;
> + return 0;
>
> return 1;
> -
> -out:
> - cb->args[2]++;
> - return 0;
> }
>
> static int sctp_ep_dump(struct sctp_endpoint *ep, void *p)
> @@ -503,12 +493,8 @@ static void sctp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
> if (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE)))
> goto done;
>
> -next:
> - cb->args[5] = 0;
> - sctp_for_each_transport(sctp_get_sock, net, cb->args[2], &commp);
> -
> - if (cb->args[5] && !sctp_sock_dump((struct sock *)cb->args[5], &commp))
> - goto next;
> + sctp_for_each_transport(sctp_sock_filter, sctp_sock_dump,
> + net, (int *)&cb->args[2], &commp);
>
> done:
> cb->args[1] = cb->args[4];
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 1b00a1e..d4730ad 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -4658,29 +4658,39 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),
> EXPORT_SYMBOL_GPL(sctp_transport_lookup_process);
>
> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),
> - struct net *net, int pos, void *p) {
> + int (*cb_done)(struct sctp_transport *, void *),
> + struct net *net, int *pos, void *p) {
> struct rhashtable_iter hti;
> - void *obj;
> - int err;
> -
> - err = sctp_transport_walk_start(&hti);
> - if (err)
> - return err;
> + struct sctp_transport *tsp;
> + int ret;
>
> - obj = sctp_transport_get_idx(net, &hti, pos + 1);
> - for (; !IS_ERR_OR_NULL(obj); obj = sctp_transport_get_next(net, &hti)) {
> - struct sctp_transport *transport = obj;
> +again:
> + ret = sctp_transport_walk_start(&hti);
> + if (ret)
> + return ret;
>
> - if (!sctp_transport_hold(transport))
> + tsp = sctp_transport_get_idx(net, &hti, *pos + 1);
> + for (; !IS_ERR_OR_NULL(tsp); tsp = sctp_transport_get_next(net, &hti)) {
> + if (!sctp_transport_hold(tsp))
> continue;
> - err = cb(transport, p);
> - sctp_transport_put(transport);
> - if (err)
> + ret = cb(tsp, p);
> + if (ret)
> break;
> + (*pos)++;
> + sctp_transport_put(tsp);
> }
> sctp_transport_walk_stop(&hti);
>
> - return err;
> + if (ret) {
> + if (cb_done && !cb_done(tsp, p)) {
> + (*pos)++;
> + sctp_transport_put(tsp);
> + goto again;
> + }
> + sctp_transport_put(tsp);
> + }
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(sctp_for_each_transport);
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: Page allocator bottleneck
From: Mel Gorman @ 2017-09-15 10:23 UTC (permalink / raw)
To: Tariq Toukan
Cc: David Miller, Jesper Dangaard Brouer, Eric Dumazet,
Alexei Starovoitov, Saeed Mahameed, Eran Ben Elisha,
Linux Kernel Network Developers, Andrew Morton, Michal Hocko,
linux-mm
In-Reply-To: <cef85936-10b2-5d76-9f97-cb03b418fd94@mellanox.com>
On Thu, Sep 14, 2017 at 07:49:31PM +0300, Tariq Toukan wrote:
> Insights:
> Major degradation between #1 and #2, not getting any close to linerate!
> Degradation is fixed between #2 and #3.
> This is because page allocator cannot stand the higher allocation rate.
> In #2, we also see that the addition of rings (cores) reduces BW (!!), as
> result of increasing congestion over shared resources.
>
Unfortunately, no surprises there.
> Congestion in this case is very clear.
> When monitored in perf top:
> 85.58% [kernel] [k] queued_spin_lock_slowpath
>
While it's not proven, the most likely candidate is the zone lock and
that should be confirmed using a call-graph profile. If so, then the
suggestion to tune to the size of the per-cpu allocator would mitigate
the problem.
> I think that page allocator issues should be discussed separately:
> 1) Rate: Increase the allocation rate on a single core.
> 2) Scalability: Reduce congestion and sync overhead between cores.
>
> This is clearly the current bottleneck in the network stack receive flow.
>
> I know about some efforts that were made in the past two years.
> For example the ones from Jesper et al.:
> - Page-pool (not accepted AFAIK).
Indeed not and it would also need driver conversion.
> - Page-allocation bulking.
Prototypes exist but it's pointless without the pool or driver
conversion so it's in the back burner for the moment.
> - Optimize order-0 allocations in Per-Cpu-Pages.
>
This had a prototype that was reverted as it must be able to cope with
both irq and noirq contexts. Unfortunately I never found the time to
revisit it but a split there to handle both would mitigate the problem.
Probably not enough to actually reach line speed though so tuning of the
per-cpu allocator sizes would still be needed. I don't know when I'll
get the chance to revisit it. I'm travelling all next week and am mostly
occupied with other work at the moment that is consuming all my
concentration.
> I am not an mm expert, but wanted to raise the issue again, to combine the
> efforts and hear from you guys about status and possible directions.
The recent effort to reduce overhead from stats will help mitigate the
problem. Finishing the page pool, the bulk allocator and converting drivers
would be the most likely successful path forward but it's currently stalled
as everyone that was previously involved is too busy.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ 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