* Re: [PATCH v2] net: dsa: qca8k: enable port flow control
From: xiaofeis @ 2019-07-27 1:15 UTC (permalink / raw)
To: Andrew Lunn; +Cc: vkoul, netdev
In-Reply-To: <20190726132919.GB18223@lunn.ch>
On 2019-07-26 21:29, Andrew Lunn wrote:
>> I didn't compile it on this tree, same code is just compiled and
>> tested on
>> kernel v4.14.
>
> For kernel development work, v4.14 is dead. It died 12th November
> 2017. It gets backports of bug fixes, but kernel developers otherwise
> don't touch it.
>
>> We are working on one google project, all the change is
>> required to upstream by Google.
>> But if I do the change based on the new type for kernel 5.3, then the
>> commit
>> can't be used directly for Google's project.
>
> So you will need to backport the change. In this case, you will have a
> very different patch in v4.14 than in mainline, due to changes like
> this. That is part of the pain in using such an old kernel.
>
> You should use the function
>
> void phy_support_asym_pause(struct phy_device *phydev);
>
> to indicate the MAC supports asym pause.
>
> Andrew
Hi Andrew
Thanks a lot, you are correct. phy_support_asym_pause is the API to do
this.
Very appreciate for all your patinet explaination and good suggestion.
Thanks
Xiaofeis
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] flow_offload: move tc indirect block to flow offload
From: Jakub Kicinski @ 2019-07-27 0:56 UTC (permalink / raw)
To: wenxu; +Cc: pablo, fw, netfilter-devel, netdev
In-Reply-To: <1564148047-6428-2-git-send-email-wenxu@ucloud.cn>
On Fri, 26 Jul 2019 21:34:05 +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> move tc indirect block to flow_offload and rename
> it to flow indirect block.The nf_tables can use the
> indr block architecture.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 00b9aab..66f89bc 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -4,6 +4,7 @@
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <net/flow_dissector.h>
> +#include <linux/rhashtable.h>
>
> struct flow_match {
> struct flow_dissector *dissector;
> @@ -366,4 +367,42 @@ static inline void flow_block_init(struct flow_block *flow_block)
> INIT_LIST_HEAD(&flow_block->cb_list);
> }
>
> +typedef int flow_indr_block_bind_cb_t(struct net_device *dev, void *cb_priv,
> + enum tc_setup_type type, void *type_data);
> +
> +struct flow_indr_block_cb {
> + struct list_head list;
> + void *cb_priv;
> + flow_indr_block_bind_cb_t *cb;
> + void *cb_ident;
> +};
> +
> +typedef void flow_indr_block_ing_cmd_t(struct net_device *dev,
> + struct flow_block *flow_block,
> + struct flow_indr_block_cb *indr_block_cb,
> + enum flow_block_command command);
> +
> +struct flow_indr_block_dev {
> + struct rhash_head ht_node;
> + struct net_device *dev;
> + unsigned int refcnt;
> + struct list_head cb_list;
> + flow_indr_block_ing_cmd_t *ing_cmd_cb;
> + struct flow_block *flow_block;
TC can only have one block per device. Now with nftables offload we can
have multiple blocks. Could you elaborate how this is solved?
> +};
^ permalink raw reply
* Re: [PATCH net-next v3 2/3] flow_offload: support get tcf block immediately
From: Jakub Kicinski @ 2019-07-27 0:52 UTC (permalink / raw)
To: wenxu; +Cc: pablo, fw, netfilter-devel, netdev
In-Reply-To: <1564148047-6428-3-git-send-email-wenxu@ucloud.cn>
On Fri, 26 Jul 2019 21:34:06 +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> Because the new flow-indr-block can't get the tcf_block
> directly.
> It provide a callback to find the tcf block immediately
> when the device register and contain a ingress block.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
Please CC people who gave you feedback on your subsequent submissions.
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 66f89bc..3b2e848 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -391,6 +391,10 @@ struct flow_indr_block_dev {
> struct flow_block *flow_block;
> };
>
> +typedef void flow_indr_get_default_block_t(struct flow_indr_block_dev *indr_dev);
> +
> +void flow_indr_set_default_block_cb(flow_indr_get_default_block_t *cb);
> +
> struct flow_indr_block_dev *flow_indr_block_dev_lookup(struct net_device *dev);
>
> int __flow_indr_block_cb_register(struct net_device *dev, void *cb_priv,
> diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
> index 9f1ae67..db8469d 100644
> --- a/net/core/flow_offload.c
> +++ b/net/core/flow_offload.c
> @@ -298,6 +298,14 @@ struct flow_indr_block_dev *
> }
> EXPORT_SYMBOL(flow_indr_block_dev_lookup);
>
> +static flow_indr_get_default_block_t *flow_indr_get_default_block;
This static variable which can only be set to the TC's callback really
is not a great API design :/
> +void flow_indr_set_default_block_cb(flow_indr_get_default_block_t *cb)
> +{
> + flow_indr_get_default_block = cb;
> +}
> +EXPORT_SYMBOL(flow_indr_set_default_block_cb);
> +
> static struct flow_indr_block_dev *flow_indr_block_dev_get(struct net_device *dev)
> {
> struct flow_indr_block_dev *indr_dev;
> @@ -312,6 +320,10 @@ static struct flow_indr_block_dev *flow_indr_block_dev_get(struct net_device *de
>
> INIT_LIST_HEAD(&indr_dev->cb_list);
> indr_dev->dev = dev;
> +
> + if (flow_indr_get_default_block)
> + flow_indr_get_default_block(indr_dev);
> +
> if (rhashtable_insert_fast(&indr_setup_block_ht, &indr_dev->ht_node,
> flow_indr_setup_block_ht_params)) {
> kfree(indr_dev);
^ permalink raw reply
* Re: [PATCH] iplink: document 'change' option to ip link
From: Matteo Croce @ 2019-07-27 0:45 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20190726215959.6312-1-stephen@networkplumber.org>
On Sat, Jul 27, 2019 at 12:00 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Add the command alias "change" to man page.
> Don't show it on usage, since it is not commonly used.
>
> Reported-off-by: Matteo Croce <mcroce@redhat.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> man/man8/ip-link.8.in | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> index 883d88077d66..a8ae72d2b097 100644
> --- a/man/man8/ip-link.8.in
> +++ b/man/man8/ip-link.8.in
> @@ -1815,6 +1815,11 @@ can move the system to an unpredictable state. The solution
> is to avoid changing several parameters with one
> .B ip link set
> call.
> +The modifier
> +.B change
> +is equivalent to
> +.BR "set" .
> +
>
> .TP
> .BI dev " DEVICE "
> --
> 2.20.1
>
Acked-by: Matteo Croce <mcroce@redhat.com>
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
From: Alexei Starovoitov @ 2019-07-27 0:34 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190726222652.GG24397@mini-arch>
On Fri, Jul 26, 2019 at 03:26:52PM -0700, Stanislav Fomichev wrote:
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > This patch changes how test output is printed out. By default, if test
> > > > had no errors, the only output will be a single line with test number,
> > > > name, and verdict at the end, e.g.:
> > > >
> > > > #31 xdp:OK
> > > >
> > > > If test had any errors, all log output captured during test execution
> > > > will be output after test completes.
> > > >
> > > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > > which case output won't be buffered and will be output immediately.
> > > >
> > > > To support this, individual tests are required to use helper methods for
> > > > logging: `test__printf()` and `test__vprintf()`.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > > >
> >
> > [...]
> >
> > > > error_cnt++;
> > > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > #define printf(...) test__printf(...) in tests.h?
> > >
> > > A bit ugly, but no need to retrain everyone to use new printf wrappers.
> >
> > I try to reduce amount of magic and surprising things, not add new
> > ones :) I also led by example and converted all current instances of
> > printf usage to test__printf, so anyone new will just copy/paste good
> > example, hopefully. Even if not, this non-buffered output will be
> > immediately obvious to anyone who just runs `sudo ./test_progs`.
>
> [..]
> > And
> > author of new test with this problem should hopefully be the first and
> > the only one to catch and fix this.
> Yeah, that is my only concern, that regular printfs will eventually
> creep in. It's already confusing to go to/from printf/printk.
>
> 2c:
>
> I'm coming from a perspective of tools/testing/selftests/kselftest.h
> which is supposed to be a generic framework with custom
> printf variants (ksft_print_msg), but I still see a bunch of tests
> calling printf :-/
>
> grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
>
> Since we don't expect regular buffered io from the tests anyway
> it might be easier just to add a bit of magic and call it a day.
I think #define printf()
is not a good style in general.
glibc functions should never be #define-d.
^ permalink raw reply
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
From: Alexei Starovoitov @ 2019-07-27 0:30 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzYoiL7XAXFdLaf5TDDas42u+jUTy2WydgmJT7WiniqOqQ@mail.gmail.com>
On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > libbpf_swap_print allows to restore previously set print function.
> > > This is useful when running many independent test with one default print
> > > function, but overriding log verbosity for particular subset of tests.
> > Can we change the return type of libbpf_set_print instead and return
> > the old function from it? Will it break ABI?
>
> Yeah, thought about that, but I wasn't sure about ABI breakage. It
> seems like it shouldn't, so I'll just change libbpf_set_print
> signature instead.
I think it's ok to change return value of libbpf_set_print() from void
to useful pointer.
This function is not marked as __attribute__((__warn_unused_result__)),
so there should be no abi issues.
Please double check by compiler perf with different gcc-s as Arnaldo's setup does.
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix erroneous multi-closing of BTF FD
From: Alexei Starovoitov @ 2019-07-27 0:24 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Network Development, Alexei Starovoitov, Daniel Borkmann,
Andrey Ignatov, Yonghong Song, Andrii Nakryiko, Kernel Team
In-Reply-To: <20190726212438.1269599-1-andriin@fb.com>
On Fri, Jul 26, 2019 at 2:25 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Libbpf stores associated BTF FD per each instance of bpf_program. When
> program is unloaded, that FD is closed. This is wrong, because leads to
> a race and possibly closing of unrelated files, if application
> simultaneously opens new files while bpf_programs are unloaded.
>
> It's also unnecessary, because struct btf "owns" that FD, and
> btf__free(), called from bpf_object__close() will close it. Thus the fix
> is to never have per-program BTF FD and fetch it from obj->btf, when
> necessary.
>
> Fixes: 2993e0515bb4 ("tools/bpf: add support to read .BTF.ext sections")
> Reported-by: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied. Thanks
^ permalink raw reply
* Re: [PATCH v2 bpf] libbpf: fix missing __WORDSIZE definition
From: Arnaldo Carvalho de Melo @ 2019-07-27 0:03 UTC (permalink / raw)
To: Andrii Nakryiko, Arnaldo Carvalho de Melo
Cc: Daniel Borkmann, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Arnaldo Carvalho de Melo, Kernel Team
In-Reply-To: <CAEf4BzYZT3fmQUuGp45+Mn6hLLyWnT2NE3PxfpD88sThX8JS_w@mail.gmail.com>
On July 26, 2019 8:01:10 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>On Fri, Jul 26, 2019 at 1:49 PM Arnaldo Carvalho de Melo
><arnaldo.melo@gmail.com> wrote:
>>
>> Em Thu, Jul 18, 2019 at 10:30:21AM -0700, Andrii Nakryiko escreveu:
>> > hashmap.h depends on __WORDSIZE being defined. It is defined by
>> > glibc/musl in different headers. It's an explicit goal for musl to
>be
>> > "non-detectable" at compilation time, so instead include glibc
>header if
>> > glibc is explicitly detected and fall back to musl header
>otherwise.
>> >
>> > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe
>internal hashmap")
>> > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>>
>> Couldn't find ths in the bpf tree, please consider applying it:
>>
>> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
>Arnaldo, I somehow got impression that you were going to pull this
>into your perf tree. Can you please confirm that it wasn't pulled into
>your tree, so that Alexei can apply it to bpf tree? Thanks!
>
I can process it, just was unsure about where it should go by,
I'll have it in my next perf/urgent pull req,
Thanks,
- Arnaldo
>
>>
>>
>> - Arnaldo
>>
>> > ---
>> > tools/lib/bpf/hashmap.h | 5 +++++
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
>> > index 03748a742146..bae8879cdf58 100644
>> > --- a/tools/lib/bpf/hashmap.h
>> > +++ b/tools/lib/bpf/hashmap.h
>> > @@ -10,6 +10,11 @@
>> >
>> > #include <stdbool.h>
>> > #include <stddef.h>
>> > +#ifdef __GLIBC__
>> > +#include <bits/wordsize.h>
>> > +#else
>> > +#include <bits/reg.h>
>> > +#endif
>> > #include "libbpf_internal.h"
>> >
>> > static inline size_t hash_bits(size_t h, int bits)
>> > --
>> > 2.17.1
>>
>> --
>>
>> - Arnaldo
^ permalink raw reply
* Re: [PATCH bpf-next 2/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call
From: Jakub Kicinski @ 2019-07-27 0:02 UTC (permalink / raw)
To: Brian Vazquez
Cc: Yonghong Song, Alexei Starovoitov, Song Liu, Brian Vazquez,
Daniel Borkmann, David S . Miller, Stanislav Fomichev,
Willem de Bruijn, Petar Penkov, Networking, bpf
In-Reply-To: <CABCgpaVB+iDGO132d9CTtC_GYiKJuuL6pe5_Krm3-THgvfMO=A@mail.gmail.com>
On Fri, 26 Jul 2019 16:36:19 -0700, Brian Vazquez wrote:
> > In bcc, we have many instances like this:
> > getting all (key value) pairs, do some analysis and output,
> > delete all keys
> >
> > The implementation typically like
> > /* to get all (key, value) pairs */
> > while(bpf_get_next_key() == 0)
> > bpf_map_lookup()
> > /* do analysis and output */
> > for (all keys)
> > bpf_map_delete()
>
> If you do that in a map that is being modified while you are doing the
> analysis and output, you will lose some new data by deleting the keys,
> right?
>
> > get_next+lookup+delete will be definitely useful.
> > batching will be even better to save the number of syscalls.
> >
> > An alternative is to do batch get_next+lookup and batch delete
> > to achieve similar goal as the above code.
>
> What I mentioned above is what it makes me think that with the
> deletion it'd be better if we perform these 3 operations at once:
> get_next+lookup+delete in a jumbo/atomic command and batch them later?
Hm. The lookup+delete are only "atomic" if we insert an RCU sync in
between right? The elements' lifetime is protected by RCU.
CPU 1 CPU 2
val = lookup(map, key)
val = lookup(map, key)
delete(map, key)
dump(val)
val->counter++
So we'd need to walk the hash table, disconnect all lists from the
buckets and splice them onto one combined list, then synchronize_rcu()
and only after that we can dump the elements and free them.
> > There is a minor difference between this approach
> > and the above get_next+lookup+delete.
> > During scanning the hash map, get_next+lookup may get less number
> > of elements compared to get_next+lookup+delete as the latter
> > may have more later-inserted hash elements after the operation
> > start. But both are inaccurate, so probably the difference
> > is minor.
> > For not changing map, looping of (get_next, lookup) and batch
> > get_next+lookup should have the same results.
>
> This is true for the api I'm presenting the only think that I was
> missing was what to do for changing maps to avoid the weird scenario
> (getting the first key due a concurrent deletion). And, in my opinion
> the way to go should be what also Willem supported: return the err to
> the caller and restart the dumping. I could do this with existing code
> just by detecting that we do provide a prev_key and got the first_key
> instead of the next_key or even implement a new function if you want
> to.
My knee jerk reaction to Willem's proposal was to nit pick that sizing
the dump space to the map's max entries doesn't guarantee all entries
will fit if an entry can be removed and re-added in a different place
while dumper proceeds.
If we keep elements disconnected from the hash array _without_
decrementing element count until synchronize_rcu() completes we have
that guarantee, but OTOH it may not be possible to add any new entries
from the datapath, so that doesn't sound great either :/
^ permalink raw reply
* Re: [PATCH bpf-next 2/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call
From: Brian Vazquez @ 2019-07-26 23:36 UTC (permalink / raw)
To: Yonghong Song
Cc: Alexei Starovoitov, Song Liu, Brian Vazquez, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, Networking, bpf
In-Reply-To: <beb513cb-2d76-30d4-6500-2892c6566a7e@fb.com>
On Thu, Jul 25, 2019 at 11:10 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/25/19 6:47 PM, Alexei Starovoitov wrote:
> > On Thu, Jul 25, 2019 at 6:24 PM Brian Vazquez <brianvv.kernel@gmail.com> wrote:
> >>
> >> On Thu, Jul 25, 2019 at 4:54 PM Alexei Starovoitov
> >> <alexei.starovoitov@gmail.com> wrote:
> >>>
> >>> On Thu, Jul 25, 2019 at 04:25:53PM -0700, Brian Vazquez wrote:
> >>>>>>> If prev_key is deleted before map_get_next_key(), we get the first key
> >>>>>>> again. This is pretty weird.
> >>>>>>
> >>>>>> Yes, I know. But note that the current scenario happens even for the
> >>>>>> old interface (imagine you are walking a map from userspace and you
> >>>>>> tried get_next_key the prev_key was removed, you will start again from
> >>>>>> the beginning without noticing it).
> >>>>>> I tried to sent a patch in the past but I was missing some context:
> >>>>>> before NULL was used to get the very first_key the interface relied in
> >>>>>> a random (non existent) key to retrieve the first_key in the map, and
> >>>>>> I was told what we still have to support that scenario.
> >>>>>
> >>>>> BPF_MAP_DUMP is slightly different, as you may return the first key
> >>>>> multiple times in the same call. Also, BPF_MAP_DUMP is new, so we
> >>>>> don't have to support legacy scenarios.
> >>>>>
> >>>>> Since BPF_MAP_DUMP keeps a list of elements. It is possible to try
> >>>>> to look up previous keys. Would something down this direction work?
> >>>>
> >>>> I've been thinking about it and I think first we need a way to detect
> >>>> that since key was not present we got the first_key instead:
> >>>>
> >>>> - One solution I had in mind was to explicitly asked for the first key
> >>>> with map_get_next_key(map, NULL, first_key) and while walking the map
> >>>> check that map_get_next_key(map, prev_key, key) doesn't return the
> >>>> same key. This could be done using memcmp.
> >>>> - Discussing with Stan, he mentioned that another option is to support
> >>>> a flag in map_get_next_key to let it know that we want an error
> >>>> instead of the first_key.
> >>>>
> >>>> After detecting the problem we also need to define what we want to do,
> >>>> here some options:
> >>>>
> >>>> a) Return the error to the caller
> >>>> b) Try with previous keys if any (which be limited to the keys that we
> >>>> have traversed so far in this dump call)
> >>>> c) continue with next entries in the map. array is easy just get the
> >>>> next valid key (starting on i+1), but hmap might be difficult since
> >>>> starting on the next bucket could potentially skip some keys that were
> >>>> concurrently added to the same bucket where key used to be, and
> >>>> starting on the same bucket could lead us to return repeated elements.
> >>>>
> >>>> Or maybe we could support those 3 cases via flags and let the caller
> >>>> decide which one to use?
> >>>
> >>> this type of indecision is the reason why I wasn't excited about
> >>> batch dumping in the first place and gave 'soft yes' when Stan
> >>> mentioned it during lsf/mm/bpf uconf.
> >>> We probably shouldn't do it.
> >>> It feels this map_dump makes api more complex and doesn't really
> >>> give much benefit to the user other than large map dump becomes faster.
> >>> I think we gotta solve this problem differently.
> >>
> >> Some users are working around the dumping problems with the existing
> >> api by creating a bpf_map_get_next_key_and_delete userspace function
> >> (see https://urldefense.proofpoint.com/v2/url?u=https-3A__www.bouncybouncy.net_blog_bpf-5Fmap-5Fget-5Fnext-5Fkey-2Dpitfalls_&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=XvNxqsDhRi62gzZ04HbLRTOFJX8X6mTuK7PZGn80akY&s=7q7beZxOJJ3Q0el8L0r-xDctedSpnEejJ6PVX1XYotQ&e= )
> >> which in my opinion is actually a good idea. The only problem with
> >> that is that calling bpf_map_get_next_key(fd, key, next_key) and then
> >> bpf_map_delete_elem(fd, key) from userspace is racing with kernel code
> >> and it might lose some information when deleting.
> >> We could then do map_dump_and_delete using that idea but in the kernel
> >> where we could better handle the racing condition. In that scenario
> >> even if we retrieve the same key it will contain different info ( the
> >> delta between old and new value). Would that work?
> >
> > you mean get_next+lookup+delete at once?
> > Sounds useful.
> > Yonghong has been thinking about batching api as well.
>
> In bcc, we have many instances like this:
> getting all (key value) pairs, do some analysis and output,
> delete all keys
>
> The implementation typically like
> /* to get all (key, value) pairs */
> while(bpf_get_next_key() == 0)
> bpf_map_lookup()
> /* do analysis and output */
> for (all keys)
> bpf_map_delete()
If you do that in a map that is being modified while you are doing the
analysis and output, you will lose some new data by deleting the keys,
right?
> get_next+lookup+delete will be definitely useful.
> batching will be even better to save the number of syscalls.
>
> An alternative is to do batch get_next+lookup and batch delete
> to achieve similar goal as the above code.
What I mentioned above is what it makes me think that with the
deletion it'd be better if we perform these 3 operations at once:
get_next+lookup+delete in a jumbo/atomic command and batch them later?
>
> There is a minor difference between this approach
> and the above get_next+lookup+delete.
> During scanning the hash map, get_next+lookup may get less number
> of elements compared to get_next+lookup+delete as the latter
> may have more later-inserted hash elements after the operation
> start. But both are inaccurate, so probably the difference
> is minor.
>
> >
> > I think if we cannot figure out how to make a batch of two commands
> > get_next + lookup to work correctly then we need to identify/invent one
> > command and make batching more generic.
>
> not 100% sure. It will be hard to define what is "correctly".
I agree, it'll be hard to define what is the right behavior.
> For not changing map, looping of (get_next, lookup) and batch
> get_next+lookup should have the same results.
This is true for the api I'm presenting the only think that I was
missing was what to do for changing maps to avoid the weird scenario
(getting the first key due a concurrent deletion). And, in my opinion
the way to go should be what also Willem supported: return the err to
the caller and restart the dumping. I could do this with existing code
just by detecting that we do provide a prev_key and got the first_key
instead of the next_key or even implement a new function if you want
to.
> For constant changing loops, not sure how to define which one
> is correct. If users have concerns, they may need to just pick one
> which gives them more comfort.
>
> > Like make one jumbo/compound/atomic command to be get_next+lookup+delete.
> > Define the semantics of this single compound command.
> > And then let batching to be a multiplier of such command.
> > In a sense that multiplier 1 or N should be have the same way.
> > No extra flags to alter the batching.
> > The high level description of the batch would be:
> > pls execute get_next,lookup,delete and repeat it N times.
> > or
> > pls execute get_next,lookup and repeat N times.
But any attempt to do get_next+lookup will have same problem with
deletions right?
I don't see how we could do it more consistent than what I'm
proposing. Let's just support one case: report an error if the
prev_key was not found instead of retrieving the first_key. Would that
work?
> > where each command action is defined to be composable.
> >
> > Just a rough idea.
> >
^ permalink raw reply
* Re: [PATCH bpf-next 06/10] selftests/bpf: add CO-RE relocs array tests
From: Andrii Nakryiko @ 2019-07-26 23:29 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Yonghong Song, Kernel Team
In-Reply-To: <20190725232633.zt6fxixq72xqwwmz@ast-mbp>
On Thu, Jul 25, 2019 at 4:26 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Jul 24, 2019 at 12:27:38PM -0700, Andrii Nakryiko wrote:
> > Add tests for various array handling/relocation scenarios.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ...
> > +
> > +#define CORE_READ(dst, src) \
> > + bpf_probe_read(dst, sizeof(*src), __builtin_preserve_access_index(src))
>
> This is the key accessor that all progs will use.
> Please split just this single macro into individual commit and add
> detailed comment about its purpose and
> what __builtin_preserve_access_index() does underneath.
I'm planning to add more powerful and flexible set of macros to
support BCC style a->b->c->d accesses using single macro. Something
like BPF_CORE_READ(&dst, sizeof(dst), a, b, c, d). I want to move
bpf_helpers.h into libbpf itself first, after some clean up. How about
I write all that at that time and for now just add this simpler
CORE_READ into bpf_helpers.h?
Relocations recorded by __builtin_preserve_access_index() are
described pretty well in patch #1, which adds bpf_offset_reloc. I'll
double check if I mention this built-in there, and if not - will add
that.
>
> > +SEC("raw_tracepoint/sys_enter")
> > +int test_core_nesting(void *ctx)
> > +{
> > + struct core_reloc_arrays *in = (void *)&data.in;
> > + struct core_reloc_arrays_output *out = (void *)&data.out;
> > +
> > + /* in->a[2] */
> > + if (CORE_READ(&out->a2, &in->a[2]))
> > + return 1;
> > + /* in->b[1][2][3] */
> > + if (CORE_READ(&out->b123, &in->b[1][2][3]))
> > + return 1;
^ permalink raw reply
* Re: memory leak in kobject_set_name_vargs (2)
From: syzbot @ 2019-07-26 23:26 UTC (permalink / raw)
To: catalin.marinas, davem, dvyukov, herbert, kuznet, kvalo,
linux-kernel, linux-mm, luciano.coelho, netdev, steffen.klassert,
syzkaller-bugs, torvalds, yoshfuji
In-Reply-To: <000000000000edcb3c058e6143d5@google.com>
syzbot has bisected this bug to:
commit 0e034f5c4bc408c943f9c4a06244415d75d7108c
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed May 18 18:51:25 2016 +0000
iwlwifi: fix mis-merge that breaks the driver
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10f955f0600000
start commit: 3bfe1fc4 Merge tag 'for-5.3/dm-changes-2' of git://git.ker..
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=12f955f0600000
console output: https://syzkaller.appspot.com/x/log.txt?x=14f955f0600000
kernel config: https://syzkaller.appspot.com/x/.config?x=dcfc65ee492509c6
dashboard link: https://syzkaller.appspot.com/bug?extid=ad8ca40ecd77896d51e2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=135cbed0600000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14dd4e34600000
Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
Fixes: 0e034f5c4bc4 ("iwlwifi: fix mis-merge that breaks the driver")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt
From: Saeed Mahameed @ 2019-07-26 23:18 UTC (permalink / raw)
To: linyunsheng@huawei.com, tanhuazhong@huawei.com,
davem@davemloft.net
Cc: lipeng321@huawei.com, yisen.zhuang@huawei.com,
salil.mehta@huawei.com, linuxarm@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <db2d081f-b892-b141-7fa5-44e66dd97eb9@huawei.com>
On Thu, 2019-07-25 at 10:05 +0800, Yunsheng Lin wrote:
> On 2019/7/25 3:24, Saeed Mahameed wrote:
> > On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> > > From: Yunsheng Lin <linyunsheng@huawei.com>
> > >
[...]
> > >
> > > +static void hclge_irq_affinity_notify(struct irq_affinity_notify
> > > *notify,
> > > + const cpumask_t *mask)
> > > +{
> > > + struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
> > > + affinity_notify);
> > > +
> > > + cpumask_copy(&hdev->affinity_mask, mask);
> > > + del_timer_sync(&hdev->service_timer);
> > > + hdev->service_timer.expires = jiffies + HZ;
> > > + add_timer_on(&hdev->service_timer, cpumask_first(&hdev-
> > > > affinity_mask));
> > > +}
> >
> > I don't see any relation between your misc irq vector and &hdev-
> > > service_timer, to me this looks like an abuse of the irq affinity
> > > API
> > to allow the user to move the service timer affinity.
>
> Hi, thanks for reviewing.
>
> hdev->service_timer is used to schedule the periodic work
> queue hdev->service_task， we want all the management work
> queue including hdev->service_task to bind to the same cpu
> to improve cache and power efficiency, it is better to move
> service timer affinity according to that.
>
> The hdev->service_task is changed to delay work queue in
> next patch " net: hns3: make hclge_service use delayed workqueue",
> So the affinity in the timer of the delay work queue is automatically
> set to the affinity of the delay work queue, we will move the
> "make hclge_service use delayed workqueue" patch before the
> "add interrupt affinity support for misc interrupt" patch, so
> we do not have to set service timer affinity explicitly.
>
> Also, There is there work queues(mbx_service_task, service_task,
> rst_service_task) in the hns3 driver, we plan to combine them
> to one or two workqueue to improve efficiency and readability.
>
So just to make it clear, you have 3 deferred works, 2 are triggered by
the misc irq and 1 is periodic, you want them all on the same core and
you want to control their affinity via the irq affinity ? for works #1
and #2 you get that for free since the irq is triggering them, but for
work #3 (the periodic one) you need to manually move it when the irq
affinity changes.
I guess i am ok with this, since moving the irq affinity isn't only
required by the periodic work but also for the other works that the irq
is actually triggering (so it is not an actual abuse, sort of .. )
^ permalink raw reply
* Re: [PATCH v2 bpf] libbpf: fix missing __WORDSIZE definition
From: Andrii Nakryiko @ 2019-07-26 23:01 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Daniel Borkmann, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Arnaldo Carvalho de Melo, Kernel Team
In-Reply-To: <20190726204937.GD24867@kernel.org>
On Fri, Jul 26, 2019 at 1:49 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Thu, Jul 18, 2019 at 10:30:21AM -0700, Andrii Nakryiko escreveu:
> > hashmap.h depends on __WORDSIZE being defined. It is defined by
> > glibc/musl in different headers. It's an explicit goal for musl to be
> > "non-detectable" at compilation time, so instead include glibc header if
> > glibc is explicitly detected and fall back to musl header otherwise.
> >
> > Fixes: e3b924224028 ("libbpf: add resizable non-thread safe internal hashmap")
> > Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Couldn't find ths in the bpf tree, please consider applying it:
>
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Arnaldo, I somehow got impression that you were going to pull this
into your perf tree. Can you please confirm that it wasn't pulled into
your tree, so that Alexei can apply it to bpf tree? Thanks!
>
>
> - Arnaldo
>
> > ---
> > tools/lib/bpf/hashmap.h | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h
> > index 03748a742146..bae8879cdf58 100644
> > --- a/tools/lib/bpf/hashmap.h
> > +++ b/tools/lib/bpf/hashmap.h
> > @@ -10,6 +10,11 @@
> >
> > #include <stdbool.h>
> > #include <stddef.h>
> > +#ifdef __GLIBC__
> > +#include <bits/wordsize.h>
> > +#else
> > +#include <bits/reg.h>
> > +#endif
> > #include "libbpf_internal.h"
> >
> > static inline size_t hash_bits(size_t h, int bits)
> > --
> > 2.17.1
>
> --
>
> - Arnaldo
^ permalink raw reply
* Re: [PATCH net-next v2] mlx4/en_netdev: allow offloading VXLAN over VLAN
From: Saeed Mahameed @ 2019-07-26 22:47 UTC (permalink / raw)
To: dcaratti@redhat.com, netdev@vger.kernel.org, davem@davemloft.net,
Tariq Toukan, pabeni@redhat.com, marcelo.leitner@gmail.com
In-Reply-To: <2beb05557960e04aa588ecc09e9ee5e5a19fc651.1564164688.git.dcaratti@redhat.com>
On Fri, 2019-07-26 at 20:18 +0200, Davide Caratti wrote:
> ConnectX-3 Pro can offload transmission of VLAN packets with VXLAN
> inside:
> enable tunnel offloads in dev->vlan_features, like it's done with
> other
> NIC drivers (e.g. be2net and ixgbe).
>
> It's no more necessary to change dev->hw_enc_features when VXLAN are
> added
> or removed, since .ndo_features_check() already checks for VXLAN
> packet
> where the UDP destination port matches the configured value. Just set
> dev->hw_enc_features when the NIC is initialized, so that overlying
> VLAN
> can correctly inherit the tunnel offload capabilities.
>
> Changes since v1:
> - avoid flipping hw_enc_features, instead of calling netdev
> notifiers,
> thanks to Saeed Mahameed
> - squash two patches into a single one
>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
^ permalink raw reply
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
From: Stanislav Fomichev @ 2019-07-26 22:26 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzaVCdHT_U+m7niJLsSmbf+M9DrFjf_PNOmQQZvuHsr9Xg@mail.gmail.com>
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > This patch changes how test output is printed out. By default, if test
> > > had no errors, the only output will be a single line with test number,
> > > name, and verdict at the end, e.g.:
> > >
> > > #31 xdp:OK
> > >
> > > If test had any errors, all log output captured during test execution
> > > will be output after test completes.
> > >
> > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > which case output won't be buffered and will be output immediately.
> > >
> > > To support this, individual tests are required to use helper methods for
> > > logging: `test__printf()` and `test__vprintf()`.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > >
>
> [...]
>
> > > error_cnt++;
> > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > #define printf(...) test__printf(...) in tests.h?
> >
> > A bit ugly, but no need to retrain everyone to use new printf wrappers.
>
> I try to reduce amount of magic and surprising things, not add new
> ones :) I also led by example and converted all current instances of
> printf usage to test__printf, so anyone new will just copy/paste good
> example, hopefully. Even if not, this non-buffered output will be
> immediately obvious to anyone who just runs `sudo ./test_progs`.
[..]
> And
> author of new test with this problem should hopefully be the first and
> the only one to catch and fix this.
Yeah, that is my only concern, that regular printfs will eventually
creep in. It's already confusing to go to/from printf/printk.
2c:
I'm coming from a perspective of tools/testing/selftests/kselftest.h
which is supposed to be a generic framework with custom
printf variants (ksft_print_msg), but I still see a bunch of tests
calling printf :-/
grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
Since we don't expect regular buffered io from the tests anyway
it might be easier just to add a bit of magic and call it a day.
> > > }
> > > out:
> > > bpf_object__close(obj);
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > index ee99368c595c..2e78217ed3fd 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
>
> [...]
^ permalink raw reply
* Re: [PATCH V2 net-next 07/11] net: hns3: adds debug messages to identify eth down cause
From: Joe Perches @ 2019-07-26 22:18 UTC (permalink / raw)
To: Saeed Mahameed, tanhuazhong@huawei.com, davem@davemloft.net
Cc: lipeng321@huawei.com, yisen.zhuang@huawei.com,
salil.mehta@huawei.com, linuxarm@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
liuyonglong@huawei.com
In-Reply-To: <a32ca755bfd69046cf89aeacbf67fd16313de768.camel@mellanox.com>
On Fri, 2019-07-26 at 22:00 +0000, Saeed Mahameed wrote:
> On Fri, 2019-07-26 at 11:24 +0800, Huazhong Tan wrote:
> > From: Yonglong Liu <liuyonglong@huawei.com>
> >
> > Some times just see the eth interface have been down/up via
> > dmesg, but can not know why the eth down. So adds some debug
> > messages to identify the cause for this.
[]
> > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> []
> > @@ -459,6 +459,10 @@ static int hns3_nic_net_open(struct net_device
> > *netdev)
> > h->ae_algo->ops->set_timer_task(priv->ae_handle, true);
> >
> > hns3_config_xps(priv);
> > +
> > + if (netif_msg_drv(h))
> > + netdev_info(netdev, "net open\n");
> > +
>
> to make sure this is only intended for debug, and to avoid repetition.
> #define hns3_dbg(__dev, format, args...) \
> ({ \
> if (netif_msg_drv(h)) \
> netdev_info(h->netdev, format, ##args); \
> })
netif_dbg(h, drv, h->netdev, "net open\n")
^ permalink raw reply
* [PATCH] gigaset: stop maintaining seperately
From: Paul Bolle @ 2019-07-26 22:05 UTC (permalink / raw)
To: David Miller
Cc: Tilman Schmidt, Hansjoerg Lipp, Arnd Bergmann, Karsten Keil,
netdev, linux-kernel
The Dutch consumer grade ISDN network will be shut down on September 1,
2019. This means I'll be converted to some sort of VOIP shortly. At that
point it would be unwise to try to maintain the gigaset driver, even for
odd fixes as I do. So I'll stop maintaining it as a seperate driver and
bump support to CAPI in staging. De facto this means the driver will be
unmaintained, since no-one seems to be working on CAPI.
I've lighty tested the hardware specific modules of this driver (bas-gigaset,
ser-gigaset, and usb-gigaset) for v5.3-rc1. The basic functionality appears to
be working. It's unclear whether anyone still cares. I'm aware of only one
person sort of using the driver a few years ago.
Thanks to Karsten Keil for the ISDN subsystems gigaset was using (I4L and
CAPI). And many thanks to Hansjoerg Lipp and Tilman Schmidt for writing and
upstreaming this driver.
Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
MAINTAINERS | 7 -------
1 file changed, 7 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..e99afbd13355 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6822,13 +6822,6 @@ F: Documentation/filesystems/gfs2*.txt
F: fs/gfs2/
F: include/uapi/linux/gfs2_ondisk.h
-GIGASET ISDN DRIVERS
-M: Paul Bolle <pebolle@tiscali.nl>
-L: gigaset307x-common@lists.sourceforge.net
-W: http://gigaset307x.sourceforge.net/
-S: Odd Fixes
-F: drivers/staging/isdn/gigaset/
-
GNSS SUBSYSTEM
M: Johan Hovold <johan@kernel.org>
T: git git://git.kernel.org/pub/scm/linux/kernel/git/johan/gnss.git
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] iplink_can: fix format output of clock with flag -details
From: Stephen Hemminger @ 2019-07-26 22:04 UTC (permalink / raw)
To: Antonio Borneo; +Cc: netdev
In-Reply-To: <20190726130609.27704-1-borneo.antonio@gmail.com>
On Fri, 26 Jul 2019 15:06:09 +0200
Antonio Borneo <borneo.antonio@gmail.com> wrote:
> The command
> ip -details link show can0
> prints in the last line the value of the clock frequency attached
> to the name of the following value "numtxqueues", e.g.
> clock 49500000numtxqueues 1 numrxqueues 1 gso_max_size
> 65536 gso_max_segs 65535
>
> Add the missing space after the clock value.
>
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Applied, but CAN looks like it doesn't support single line output correctly.
^ permalink raw reply
* Re: [PATCH bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
From: Stanislav Fomichev @ 2019-07-26 22:03 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzZRhHTo+vUFkmLnjPxTL8oi6Fi0zrhvhA6JbY_afU3_Nw@mail.gmail.com>
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:25 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > Add ability to specify either test number or test name substring to
> > > narrow down a set of test to run.
> > >
> > > Usage:
> > > sudo ./test_progs -n 1
> > > sudo ./test_progs -t attach_probe
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
> > > 1 file changed, 39 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > index eea88ba59225..6e04b9f83777 100644
> > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > @@ -4,6 +4,7 @@
>
> [...]
>
> > >
> > > static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > > {
> > > struct test_env *env = state->input;
> > >
> > > switch (key) {
> > [..]
> > > + case ARG_TEST_NUM: {
> > > + int test_num;
> > > +
> > > + errno = 0;
> > > + test_num = strtol(arg, NULL, 10);
> > > + if (errno)
> > > + return -errno;
> > > + env->test_num_selector = test_num;
> > > + break;
> > > + }
> > Do you think it's really useful? I agree about running by name (I
>
> Special request from Alexei :) But in one of the follow up patches, I
> extended this to allow to specify arbitrary subset of tests, e.g.:
> 1,2,5-10,7-8. So in that regard, it's more powerful than selecting by
> name and gives you ultimate freedom.
I guess I didn't read the series close enough; that '1,2,3' mode does seem
quite useful indeed!
>
> > usually used grep -v in the Makefile :-), but I'm not sure about running
> > by number.
> >
> > Or is the idea is that you can just copy-paste this number from the
> > test_progs output to rerun the tests? In this case, why not copy-paste
> > the name instead?
>
> Both were simple to support, I didn't want to dictate one right way to
> do this :)
>
> >
> > > + case ARG_TEST_NAME:
> > > + env->test_name_selector = arg;
> > > + break;
> > > case ARG_VERIFIER_STATS:
> > > env->verifier_stats = true;
> > > break;
> > > @@ -223,7 +248,7 @@ int main(int argc, char **argv)
> > > .parser = parse_arg,
> > > .doc = argp_program_doc,
> > > };
> > > - const struct prog_test_def *def;
> > > + struct prog_test_def *test;
> > > int err, i;
> > >
> > > err = argp_parse(&argp, argc, argv, 0, NULL, &env);
> > > @@ -237,8 +262,18 @@ int main(int argc, char **argv)
> > > verifier_stats = env.verifier_stats;
> > >
> > > for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
> > > - def = &prog_test_defs[i];
> > > - def->run_test();
> > > + test = &prog_test_defs[i];
> > > +
> > > + test->test_num = i + 1;
> > > +
> > > + if (env.test_num_selector >= 0 &&
> > > + test->test_num != env.test_num_selector)
> > > + continue;
> > > + if (env.test_name_selector &&
> > > + !strstr(test->test_name, env.test_name_selector))
> > > + continue;
> > > +
> > > + test->run_test();
> > > }
> > >
> > > printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
> > > --
> > > 2.17.1
> > >
^ permalink raw reply
* Re: [PATCH v2] iproute2: devlink: port from sys/queue.h to list.h
From: Stephen Hemminger @ 2019-07-26 22:02 UTC (permalink / raw)
To: Sergei Trofimovich; +Cc: netdev
In-Reply-To: <20190726210105.25458-1-slyfox@gentoo.org>
On Fri, 26 Jul 2019 22:01:05 +0100
Sergei Trofimovich <slyfox@gentoo.org> wrote:
> sys/queue.h does not exist on linux-musl targets and fails build as:
>
> devlink.c:28:10: fatal error: sys/queue.h: No such file or directory
> 28 | #include <sys/queue.h>
> | ^~~~~~~~~~~~~
>
> The change ports to list.h API and drops dependency of 'sys/queue.h'.
> The API maps one-to-one.
>
> Build-tested on linux-musl and linux-glibc.
>
> Bug: https://bugs.gentoo.org/690486
> CC: Stephen Hemminger <stephen@networkplumber.org>
> CC: netdev@vger.kernel.org
> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
Looks good, thanks for following through.
Applied.
^ permalink raw reply
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
From: Stanislav Fomichev @ 2019-07-26 22:01 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzYDvZENJqrT0KKpHbfHNCdObB9p4ZcJqQj3+rM_1ESF3g@mail.gmail.com>
On 07/26, Andrii Nakryiko wrote:
> On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 07/26, Andrii Nakryiko wrote:
> > > Apprently listing header as a normal dependency for a binary output
> > > makes it go through compilation as if it was C code. This currently
> > > works without a problem, but in subsequent commits causes problems for
> > > differently generated test.h for test_progs. Marking those headers as
> > > order-only dependency solves the issue.
> > Are you sure it will not result in a situation where
> > test_progs/test_maps is not regenerated if tests.h is updated.
> >
> > If I read the following doc correctly, order deps make sense for
> > directories only:
> > https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> >
> > Can you maybe double check it with:
> > * make
> > * add new prog_tests/test_something.c
> > * make
> > to see if the binary is regenerated with test_something.c?
>
> Yeah, tested that, it triggers test_progs rebuild.
>
> Ordering is still preserved, because test.h is dependency of
> test_progs.c, which is dependency of test_progs binary, so that's why
> it works.
>
> As to why .h file is compiled as C file, I have no idea and ideally
> that should be fixed somehow.
I guess that's because it's a prerequisite and we have a target that
puts all prerequisites when calling CC:
test_progs: a.c b.c tests.h
gcc a.c b.c tests.h -o test_progs
So gcc compiles each input file.
I'm not actually sure why default dependency system that uses 'gcc -M'
is not working for us (see scripts/Kbuild.include) and we need to manually
add tests.h dependency. But that's outside of the scope..
> I also started with just removing header as dependency completely
> (because it's indirect dependency of test_progs.c), but that broke the
> build logic. Dunno, too much magic... This works, tested many-many
> times, so I was satisfied enough :)
Yeah, that's my only concern, too much magic already and we add
quite a bit more.
> > Maybe fix the problem of header compilation by having '#ifndef
> > DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
>
> That's ugly, I'd like to avoid doing that.
That's your call, but I'm not sure what's uglier: complicating already
complex make rules or making a header self contained.
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > > tools/testing/selftests/bpf/Makefile | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 11c9c62c3362..bb66cc4a7f34 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> > > PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> > > test_progs.c: $(PROG_TESTS_H)
> > > $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> > > -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> > > +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> > > $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> > > $(shell ( cd prog_tests/; \
> > > echo '/* Generated header, do not edit */'; \
> > > @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> > > MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> > > test_maps.c: $(MAP_TESTS_H)
> > > $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> > > -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> > > +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> > > $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> > > $(shell ( cd map_tests/; \
> > > echo '/* Generated header, do not edit */'; \
> > > @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> > > VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> > > test_verifier.c: $(VERIFIER_TESTS_H)
> > > $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> > > -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> > > +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> > > $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> > > $(shell ( cd verifier/; \
> > > echo '/* Generated header, do not edit */'; \
> > > --
> > > 2.17.1
> > >
^ permalink raw reply
* Re: [PATCH V2 net-next 07/11] net: hns3: adds debug messages to identify eth down cause
From: Saeed Mahameed @ 2019-07-26 22:00 UTC (permalink / raw)
To: tanhuazhong@huawei.com, davem@davemloft.net
Cc: lipeng321@huawei.com, yisen.zhuang@huawei.com,
salil.mehta@huawei.com, linuxarm@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
liuyonglong@huawei.com
In-Reply-To: <1564111502-15504-8-git-send-email-tanhuazhong@huawei.com>
On Fri, 2019-07-26 at 11:24 +0800, Huazhong Tan wrote:
> From: Yonglong Liu <liuyonglong@huawei.com>
>
> Some times just see the eth interface have been down/up via
> dmesg, but can not know why the eth down. So adds some debug
> messages to identify the cause for this.
>
> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 24
> ++++++++++++++++++++
> drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 26
> ++++++++++++++++++++++
> .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 14 ++++++++++++
> 3 files changed, 64 insertions(+)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 4d58c53..2e30cfa 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -459,6 +459,10 @@ static int hns3_nic_net_open(struct net_device
> *netdev)
> h->ae_algo->ops->set_timer_task(priv->ae_handle, true);
>
> hns3_config_xps(priv);
> +
> + if (netif_msg_drv(h))
> + netdev_info(netdev, "net open\n");
> +
to make sure this is only intended for debug, and to avoid repetition.
#define hns3_dbg(__dev, format, args...) \
({ \
if (netif_msg_drv(h)) \
netdev_info(h->netdev, format, ##args); \
})
#endif
^ permalink raw reply
* [PATCH] iplink: document 'change' option to ip link
From: Stephen Hemminger @ 2019-07-26 21:59 UTC (permalink / raw)
To: mcroce; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20190724191218.11757-1-mcroce@redhat.com>
Add the command alias "change" to man page.
Don't show it on usage, since it is not commonly used.
Reported-off-by: Matteo Croce <mcroce@redhat.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
man/man8/ip-link.8.in | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 883d88077d66..a8ae72d2b097 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -1815,6 +1815,11 @@ can move the system to an unpredictable state. The solution
is to avoid changing several parameters with one
.B ip link set
call.
+The modifier
+.B change
+is equivalent to
+.BR "set" .
+
.TP
.BI dev " DEVICE "
--
2.20.1
^ permalink raw reply related
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
From: Andrii Nakryiko @ 2019-07-26 21:51 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <20190726213104.GD24397@mini-arch>
On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > This patch changes how test output is printed out. By default, if test
> > had no errors, the only output will be a single line with test number,
> > name, and verdict at the end, e.g.:
> >
> > #31 xdp:OK
> >
> > If test had any errors, all log output captured during test execution
> > will be output after test completes.
> >
> > It's possible to force output of log with `-v` (`--verbose`) option, in
> > which case output won't be buffered and will be output immediately.
> >
> > To support this, individual tests are required to use helper methods for
> > logging: `test__printf()` and `test__vprintf()`.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > 12 files changed, 173 insertions(+), 73 deletions(-)
> >
[...]
> > error_cnt++;
> > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> #define printf(...) test__printf(...) in tests.h?
>
> A bit ugly, but no need to retrain everyone to use new printf wrappers.
I try to reduce amount of magic and surprising things, not add new
ones :) I also led by example and converted all current instances of
printf usage to test__printf, so anyone new will just copy/paste good
example, hopefully. Even if not, this non-buffered output will be
immediately obvious to anyone who just runs `sudo ./test_progs`. And
author of new test with this problem should hopefully be the first and
the only one to catch and fix this.
>
> > }
> > out:
> > bpf_object__close(obj);
> > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > index ee99368c595c..2e78217ed3fd 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
[...]
^ 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