* Re: [PATCH v2 2/9] rcu: Add support for consolidated-RCU reader checking
From: Paul E. McKenney @ 2019-07-16 18:22 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
Josh Triplett, keescook, kernel-hardening, kernel-team,
Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-3-joel@joelfernandes.org>
On Fri, Jul 12, 2019 at 01:00:17PM -0400, Joel Fernandes (Google) wrote:
> This patch adds support for checking RCU reader sections in list
> traversal macros. Optionally, if the list macro is called under SRCU or
> other lock/mutex protection, then appropriate lockdep expressions can be
> passed to make the checks pass.
>
> Existing list_for_each_entry_rcu() invocations don't need to pass the
> optional fourth argument (cond) unless they are under some non-RCU
> protection and needs to make lockdep check pass.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
If you fold in the checks for extra parameters, I will take this
one and also 1/9.
Thanx, Paul
> ---
> include/linux/rculist.h | 28 +++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 14 ++++++++++++++
> 4 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..1048160625bb 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> */
> #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next)))
>
> +/*
> + * Check during list traversal that we are within an RCU reader
> + */
> +
> +#ifdef CONFIG_PROVE_RCU_LIST
> +#define __list_check_rcu(dummy, cond, ...) \
> + ({ \
> + RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> + "RCU-list traversed in non-reader section!"); \
> + })
> +#else
> +#define __list_check_rcu(dummy, cond, ...) ({})
> +#endif
> +
> /*
> * Insert a new entry between two known consecutive entries.
> *
> @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the list_head within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as list_add_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define list_for_each_entry_rcu(pos, head, member) \
> - for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> - &pos->member != (head); \
> +#define list_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> + &pos->member != (head); \
> pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>
> /**
> @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> * @pos: the type * to use as a loop cursor.
> * @head: the head for your list.
> * @member: the name of the hlist_node within the struct.
> + * @cond: optional lockdep expression if called from non-RCU protection.
> *
> * This list-traversal primitive may safely run concurrently with
> * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> * as long as the traversal is guarded by rcu_read_lock().
> */
> -#define hlist_for_each_entry_rcu(pos, head, member) \
> - for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> +#define hlist_for_each_entry_rcu(pos, head, member, cond...) \
> + for (__list_check_rcu(dummy, ## cond, 0), \
> + pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> typeof(*(pos)), member); \
> pos; \
> pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 922bb6848813..712b464ab960 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -223,6 +223,7 @@ int debug_lockdep_rcu_enabled(void);
> int rcu_read_lock_held(void);
> int rcu_read_lock_bh_held(void);
> int rcu_read_lock_sched_held(void);
> +int rcu_read_lock_any_held(void);
>
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> @@ -243,6 +244,12 @@ static inline int rcu_read_lock_sched_held(void)
> {
> return !preemptible();
> }
> +
> +static inline int rcu_read_lock_any_held(void)
> +{
> + return !preemptible();
> +}
> +
> #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> #ifdef CONFIG_PROVE_RCU
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 0ec7d1d33a14..b20d0e2903d1 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -7,6 +7,17 @@ menu "RCU Debugging"
> config PROVE_RCU
> def_bool PROVE_LOCKING
>
> +config PROVE_RCU_LIST
> + bool "RCU list lockdep debugging"
> + depends on PROVE_RCU
> + default n
> + help
> + Enable RCU lockdep checking for list usages. By default it is
> + turned off since there are several list RCU users that still
> + need to be converted to pass a lockdep expression. To prevent
> + false-positive splats, we keep it default disabled but once all
> + users are converted, we can remove this config option.
> +
> config TORTURE_TEST
> tristate
> default n
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index bb961cd89e76..0cc7be0fb6b5 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -294,6 +294,20 @@ int rcu_read_lock_bh_held(void)
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> +int rcu_read_lock_any_held(void)
> +{
> + if (!debug_lockdep_rcu_enabled())
> + return 1;
> + if (!rcu_is_watching())
> + return 0;
> + if (!rcu_lockdep_current_cpu_online())
> + return 0;
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> +}
> +EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
> +
> #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> /**
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-16 18:28 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190716182642.GB22819@linux.ibm.com>
On Tue, Jul 16, 2019 at 11:26:42AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> > The rcu/sync code was doing its own check whether we are in a reader
> > section. With RCU consolidating flavors and the generic helper added in
> > this series, this is no longer need. We can just use the generic helper
> > and it results in a nice cleanup.
> >
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> This needs to be forward-ported to current mainline. (Or, I believe
> equivalently for this file, to branch "dev" of -rcu.)
>
> Especially given that you have Oleg's Ack, I would be happy to
> take the forward-ported version.
Never mind, I am one version behind. Apologies for the noise!
Thanx, Paul
> > ---
> > Please note: Only build and boot tested this particular patch so far.
> >
> > include/linux/rcu_sync.h | 5 ++---
> > kernel/rcu/sync.c | 22 ----------------------
> > 2 files changed, 2 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> > index 6fc53a1345b3..c954f1efc919 100644
> > --- a/include/linux/rcu_sync.h
> > +++ b/include/linux/rcu_sync.h
> > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *);
> > */
> > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> > {
> > -#ifdef CONFIG_PROVE_RCU
> > - rcu_sync_lockdep_assert(rsp);
> > -#endif
> > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> > + "suspicious rcu_sync_is_idle() usage");
> > return !rsp->gp_state; /* GP_IDLE */
> > }
> >
> > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> > index a8304d90573f..535e02601f56 100644
> > --- a/kernel/rcu/sync.c
> > +++ b/kernel/rcu/sync.c
> > @@ -10,37 +10,25 @@
> > #include <linux/rcu_sync.h>
> > #include <linux/sched.h>
> >
> > -#ifdef CONFIG_PROVE_RCU
> > -#define __INIT_HELD(func) .held = func,
> > -#else
> > -#define __INIT_HELD(func)
> > -#endif
> > -
> > static const struct {
> > void (*sync)(void);
> > void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
> > void (*wait)(void);
> > -#ifdef CONFIG_PROVE_RCU
> > - int (*held)(void);
> > -#endif
> > } gp_ops[] = {
> > [RCU_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_held)
> > },
> > [RCU_SCHED_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_sched_held)
> > },
> > [RCU_BH_SYNC] = {
> > .sync = synchronize_rcu,
> > .call = call_rcu,
> > .wait = rcu_barrier,
> > - __INIT_HELD(rcu_read_lock_bh_held)
> > },
> > };
> >
> > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> >
> > #define rss_lock gp_wait.lock
> >
> > -#ifdef CONFIG_PROVE_RCU
> > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
> > -{
> > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
> > - "suspicious rcu_sync_is_idle() usage");
> > -}
> > -
> > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert);
> > -#endif
> > -
> > /**
> > * rcu_sync_init() - Initialize an rcu_sync structure
> > * @rsp: Pointer to rcu_sync structure to be initialized
> > --
> > 2.22.0.510.g264f2c817a-goog
> >
^ permalink raw reply
* Re: [PATCH v2 3/9] rcu/sync: Remove custom check for reader-section
From: Paul E. McKenney @ 2019-07-16 18:26 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo, Thomas Gleixner,
will, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190712170024.111093-4-joel@joelfernandes.org>
On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote:
> The rcu/sync code was doing its own check whether we are in a reader
> section. With RCU consolidating flavors and the generic helper added in
> this series, this is no longer need. We can just use the generic helper
> and it results in a nice cleanup.
>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
This needs to be forward-ported to current mainline. (Or, I believe
equivalently for this file, to branch "dev" of -rcu.)
Especially given that you have Oleg's Ack, I would be happy to
take the forward-ported version.
Thanx, Paul
> ---
> Please note: Only build and boot tested this particular patch so far.
>
> include/linux/rcu_sync.h | 5 ++---
> kernel/rcu/sync.c | 22 ----------------------
> 2 files changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index 6fc53a1345b3..c954f1efc919 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *);
> */
> static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> {
> -#ifdef CONFIG_PROVE_RCU
> - rcu_sync_lockdep_assert(rsp);
> -#endif
> + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
> + "suspicious rcu_sync_is_idle() usage");
> return !rsp->gp_state; /* GP_IDLE */
> }
>
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index a8304d90573f..535e02601f56 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -10,37 +10,25 @@
> #include <linux/rcu_sync.h>
> #include <linux/sched.h>
>
> -#ifdef CONFIG_PROVE_RCU
> -#define __INIT_HELD(func) .held = func,
> -#else
> -#define __INIT_HELD(func)
> -#endif
> -
> static const struct {
> void (*sync)(void);
> void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
> void (*wait)(void);
> -#ifdef CONFIG_PROVE_RCU
> - int (*held)(void);
> -#endif
> } gp_ops[] = {
> [RCU_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_held)
> },
> [RCU_SCHED_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_sched_held)
> },
> [RCU_BH_SYNC] = {
> .sync = synchronize_rcu,
> .call = call_rcu,
> .wait = rcu_barrier,
> - __INIT_HELD(rcu_read_lock_bh_held)
> },
> };
>
> @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
>
> #define rss_lock gp_wait.lock
>
> -#ifdef CONFIG_PROVE_RCU
> -void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
> -{
> - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(),
> - "suspicious rcu_sync_is_idle() usage");
> -}
> -
> -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert);
> -#endif
> -
> /**
> * rcu_sync_init() - Initialize an rcu_sync structure
> * @rsp: Pointer to rcu_sync structure to be initialized
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v12 1/5] can: m_can: Create a m_can platform framework
From: Dan Murphy @ 2019-07-16 18:23 UTC (permalink / raw)
To: wg, mkl, davem; +Cc: linux-can, netdev, linux-kernel
In-Reply-To: <dbb7bdef-820d-5dcc-d7b5-a82bc1b076fb@ti.com>
Hello
On 5/15/19 3:54 PM, Dan Murphy wrote:
> Marc
>
> On 5/9/19 11:11 AM, Dan Murphy wrote:
>> Create a m_can platform framework that peripheral
>> devices can register to and use common code and register sets.
>> The peripheral devices may provide read/write and configuration
>> support of the IP.
>>
>> Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v12 - Update the m_can_read/write functions to create a backtrace if the callback
>> pointer is NULL. - https://lore.kernel.org/patchwork/patch/1052302/
>>
> Is this able to be merged now?
Is there anyone out there maintaining this sub system?
Dan
> Dan
>
> <snip>
^ permalink raw reply
* Re: [RFC bpf-next 0/8] bpf: accelerate insn patching speed
From: Andrii Nakryiko @ 2019-07-16 17:49 UTC (permalink / raw)
To: Jiong Wang
Cc: Alexei Starovoitov, Daniel Borkmann, Edward Cree, Naveen N. Rao,
Andrii Nakryiko, Jakub Kicinski, bpf, Networking, oss-drivers,
Yonghong Song
In-Reply-To: <87wogitlbi.fsf@netronome.com>
On Tue, Jul 16, 2019 at 1:50 AM Jiong Wang <jiong.wang@netronome.com> wrote:
>
>
> Andrii Nakryiko writes:
>
> > On Mon, Jul 15, 2019 at 2:21 AM Jiong Wang <jiong.wang@netronome.com> wrote:
> >>
> >>
> >> Andrii Nakryiko writes:
> >>
> >> > On Thu, Jul 11, 2019 at 4:22 AM Jiong Wang <jiong.wang@netronome.com> wrote:
> >> >>
> >> >>
> >> >> Andrii Nakryiko writes:
> >> >>
> >> >> > On Thu, Jul 4, 2019 at 2:31 PM Jiong Wang <jiong.wang@netronome.com> wrote:
> >> >> >>
> >> >> >> This is an RFC based on latest bpf-next about acclerating insn patching
> >> >> >> speed, it is now near the shape of final PATCH set, and we could see the
> >> >> >> changes migrating to list patching would brings, so send out for
> >> >> >> comments. Most of the info are in cover letter. I splitted the code in a
> >> >> >> way to show API migration more easily.
> >> >> >
> >> >> >
> >> >> > Hey Jiong,
> >> >> >
> >> >> >
> >> >> > Sorry, took me a while to get to this and learn more about instruction
> >> >> > patching. Overall this looks good and I think is a good direction.
> >> >> > I'll post high-level feedback here, and some more
> >> >> > implementation-specific ones in corresponding patches.
> >> >>
> >> >> Great, thanks very much for the feedbacks. Most of your feedbacks are
> >> >> hitting those pain points I exactly had ran into. For some of them, I
> >> >> thought similar solutions like yours, but failed due to various
> >> >> reasons. Let's go through them again, I could have missed some important
> >> >> things.
> >> >>
> >> >> Please see my replies below.
> >> >
> >> > Thanks for thoughtful reply :)
> >> >
> >> >>
> >> >> >>
> >> >> >> Test Results
> >> >> >> ===
> >> >> >> - Full pass on test_verifier/test_prog/test_prog_32 under all three
> >> >> >> modes (interpreter, JIT, JIT with blinding).
> >> >> >>
> >> >> >> - Benchmarking shows 10 ~ 15x faster on medium sized prog, and reduce
> >> >> >> patching time from 5100s (nearly one and a half hour) to less than
> >> >> >> 0.5s for 1M insn patching.
> >> >> >>
> >> >> >> Known Issues
> >> >> >> ===
> >> >> >> - The following warning is triggered when running scale test which
> >> >> >> contains 1M insns and patching:
> >> >> >> warning of mm/page_alloc.c:4639 __alloc_pages_nodemask+0x29e/0x330
> >> >> >>
> >> >> >> This is caused by existing code, it can be reproduced on bpf-next
> >> >> >> master with jit blinding enabled, then run scale unit test, it will
> >> >> >> shown up after half an hour. After this set, patching is very fast, so
> >> >> >> it shows up quickly.
> >> >> >>
> >> >> >> - No line info adjustment support when doing insn delete, subprog adj
> >> >> >> is with bug when doing insn delete as well. Generally, removal of insns
> >> >> >> could possibly cause remove of entire line or subprog, therefore
> >> >> >> entries of prog->aux->linfo or env->subprog needs to be deleted. I
> >> >> >> don't have good idea and clean code for integrating this into the
> >> >> >> linearization code at the moment, will do more experimenting,
> >> >> >> appreciate ideas and suggestions on this.
> >> >> >
> >> >> > Is there any specific problem to detect which line info to delete? Or
> >> >> > what am I missing besides careful implementation?
> >> >>
> >> >> Mostly line info and subprog info are range info which covers a range of
> >> >> insns. Deleting insns could causing you adjusting the range or removing one
> >> >> range entirely. subprog info could be fully recalcuated during
> >> >> linearization while line info I need some careful implementation and I
> >> >> failed to have clean code for this during linearization also as said no
> >> >> unit tests to help me understand whether the code is correct or not.
> >> >>
> >> >
> >> > Ok, that's good that it's just about clean implementation. Try to
> >> > implement it as clearly as possible. Then post it here, and if it can
> >> > be improved someone (me?) will try to help to clean it up further.
> >> >
> >> > Not a big expert on line info, so can't comment on that,
> >> > unfortunately. Maybe Yonghong can chime in (cc'ed)
> >> >
> >> >
> >> >> I will described this latter, spent too much time writing the following
> >> >> reply. Might worth an separate discussion thread.
> >> >>
> >> >> >>
> >> >> >> Insn delete doesn't happen on normal programs, for example Cilium
> >> >> >> benchmarks, and happens rarely on test_progs, so the test coverage is
> >> >> >> not good. That's also why this RFC have a full pass on selftest with
> >> >> >> this known issue.
> >> >> >
> >> >> > I hope you'll add test for deletion (and w/ corresponding line info)
> >> >> > in final patch set :)
> >> >>
> >> >> Will try. Need to spend some time on BTF format.
> >> >> >
> >> >> >>
> >> >> >> - Could further use mem pool to accelerate the speed, changes are trivial
> >> >> >> on top of this RFC, and could be 2x extra faster. Not included in this
> >> >> >> RFC as reducing the algo complexity from quadratic to linear of insn
> >> >> >> number is the first step.
> >> >> >
> >> >> > Honestly, I think that would add more complexity than necessary, and I
> >> >> > think we can further speed up performance without that, see below.
> >> >> >
> >> >> >>
> >> >> >> Background
> >> >> >> ===
> >> >> >> This RFC aims to accelerate BPF insn patching speed, patching means expand
> >> >> >> one bpf insn at any offset inside bpf prog into a set of new insns, or
> >> >> >> remove insns.
> >> >> >>
> >> >> >> At the moment, insn patching is quadratic of insn number, this is due to
> >> >> >> branch targets of jump insns needs to be adjusted, and the algo used is:
> >> >> >>
> >> >> >> for insn inside prog
> >> >> >> patch insn + regeneate bpf prog
> >> >> >> for insn inside new prog
> >> >> >> adjust jump target
> >> >> >>
> >> >> >> This is causing significant time spending when a bpf prog requires large
> >> >> >> amount of patching on different insns. Benchmarking shows it could take
> >> >> >> more than half minutes to finish patching when patching number is more
> >> >> >> than 50K, and the time spent could be more than one hour when patching
> >> >> >> number is around 1M.
> >> >> >>
> >> >> >> 15000 : 3s
> >> >> >> 45000 : 29s
> >> >> >> 95000 : 125s
> >> >> >> 195000 : 712s
> >> >> >> 1000000 : 5100s
> >> >> >>
> >> >> >> This RFC introduces new patching infrastructure. Before doing insn
> >> >> >> patching, insns in bpf prog are turned into a singly linked list, insert
> >> >> >> new insns just insert new list node, delete insns just set delete flag.
> >> >> >> And finally, the list is linearized back into array, and branch target
> >> >> >> adjustment is done for all jump insns during linearization. This algo
> >> >> >> brings the time complexity from quadratic to linear of insn number.
> >> >> >>
> >> >> >> Benchmarking shows the new patching infrastructure could be 10 ~ 15x faster
> >> >> >> on medium sized prog, and for a 1M patching it reduce the time from 5100s
> >> >> >> to less than 0.5s.
> >> >> >>
> >> >> >> Patching API
> >> >> >> ===
> >> >> >> Insn patching could happen on two layers inside BPF. One is "core layer"
> >> >> >> where only BPF insns are patched. The other is "verification layer" where
> >> >> >> insns have corresponding aux info as well high level subprog info, so
> >> >> >> insn patching means aux info needs to be patched as well, and subprog info
> >> >> >> needs to be adjusted. BPF prog also has debug info associated, so line info
> >> >> >> should always be updated after insn patching.
> >> >> >>
> >> >> >> So, list creation, destroy, insert, delete is the same for both layer,
> >> >> >> but lineration is different. "verification layer" patching require extra
> >> >> >> work. Therefore the patch APIs are:
> >> >> >>
> >> >> >> list creation: bpf_create_list_insn
> >> >> >> list patch: bpf_patch_list_insn
> >> >> >> list pre-patch: bpf_prepatch_list_insn
> >> >> >
> >> >> > I think pre-patch name is very confusing, until I read full
> >> >> > description I couldn't understand what it's supposed to be used for.
> >> >> > Speaking of bpf_patch_list_insn, patch is also generic enough to leave
> >> >> > me wondering whether instruction buffer is inserted after instruction,
> >> >> > or instruction is replaced with a bunch of instructions.
> >> >> >
> >> >> > So how about two more specific names:
> >> >> > bpf_patch_list_insn -> bpf_list_insn_replace (meaning replace given
> >> >> > instruction with a list of patch instructions)
> >> >> > bpf_prepatch_list_insn -> bpf_list_insn_prepend (well, I think this
> >> >> > one is pretty clear).
> >> >>
> >> >> My sense on English word is not great, will switch to above which indeed
> >> >> reads more clear.
> >> >>
> >> >> >> list lineration (core layer): prog = bpf_linearize_list_insn(prog, list)
> >> >> >> list lineration (veri layer): env = verifier_linearize_list_insn(env, list)
> >> >> >
> >> >> > These two functions are both quite involved, as well as share a lot of
> >> >> > common code. I'd rather have one linearize instruction, that takes env
> >> >> > as an optional parameter. If env is specified (which is the case for
> >> >> > all cases except for constant blinding pass), then adjust aux_data and
> >> >> > subprogs along the way.
> >> >>
> >> >> Two version of lineration and how to unify them was a painpoint to me. I
> >> >> thought to factor out some of the common code out, but it actually doesn't
> >> >> count much, the final size counting + insnsi resize parts are the same,
> >> >> then things start to diverge since the "Copy over insn" loop.
> >> >>
> >> >> verifier layer needs to copy and initialize aux data etc. And jump
> >> >> relocation is different. At core layer, the use case is JIT blinding which
> >> >> could expand an jump_imm insn into a and/or/jump_reg sequence, and the
> >> >
> >> > Sorry, I didn't get what "could expand an jump_imm insn into a
> >> > and/or/jump_reg sequence", maybe you can clarify if I'm missing
> >> > something.
> >> >
> >> > But from your cover letter description, core layer has no jumps at
> >> > all, while verifier has jumps inside patch buffer. So, if you support
> >> > jumps inside of patch buffer, it will automatically work for core
> >> > layer. Or what am I missing?
> >>
> >> I meant in core layer (JIT blinding), there is the following patching:
> >>
> >> input:
> >> insn 0 insn 0
> >> insn 1 insn 1
> >> jmp_imm >> mov_imm \
> >> insn 2 xor_imm insn seq expanded from jmp_imm
> >> insn 3 jmp_reg /
> >> insn 2
> >> insn 3
> >>
> >>
> >> jmp_imm is the insn that will be patched, and the actually transformation
> >> is to expand it into mov_imm/xor_imm/jmp_reg sequence. "jmp_reg", sitting
> >> at the end of the patch buffer, must jump to the same destination as the
> >> original jmp_imm, so "jmp_reg" is an insn inside patch buffer but should
> >> be relocated, and the jump destination is outside of patch buffer.
> >
> >
> > Ok, great, thanks for explaining, yeah it's definitely something that
> > we should be able to support. BUT. It got me thinking a bit more and I
> > think I have simpler and more elegant solution now, again, supporting
> > both core-layer and verifier-layer operations.
> >
> > struct bpf_patchable_insn {
> > struct bpf_patchable_insn *next;
> > struct bpf_insn insn;
> > int orig_idx; /* original non-patched index */
> > int new_idx; /* new index, will be filled only during linearization */
> > };
> >
> > struct bpf_patcher {
> > /* dummy head node of a chain of patchable instructions */
> > struct bpf_patchable_insn insn_head;
> > /* dynamic array of size(original instruction count)
> > * this is a map from original instruction index to a first
> > * patchable instruction that replaced that instruction (or
> > * just original instruction as bpf_patchable_insn).
> > */
> > int *orig_idx_to_patchable_insn;
> > int cnt;
> > };
> >
> > Few points, but it should be pretty clear just from comments and definitions:
> > 1. When you created bpf_patcher, you create patchabe_insn list, fill
> > orig_idx_to_patchable_insn map to store proper pointers. This array is
> > NEVER changed after that.
> > 2. When replacing instruction, you re-use struct bpf_patchable_insn
> > for first patched instruction, then append after that (not prepend to
> > next instruction to not disrupt orig_idx -> patchable_insn mapping).
> > 3. During linearizations, you first traverse the chain of instructions
> > and trivially assing new_idxs.
> > 4. No need for patchabe_insn->target anymore. All jumps use relative
> > instruction offsets, right?
>
> Yes, all jumps are pc-relative.
>
> > So when you need to determine new
> > instruction index during linearization, you just do (after you
> > calculated new instruction indicies):
> >
> > func adjust_jmp(struct bpf_patcher* patcher, struct bpf_patchable_insn *insn) {
> > int old_jmp_idx = insn->orig_idx + jmp_offset_of(insn->insn);
> > int new_jmp_idx = patcher->orig_idx_to_patchable_insn[old_jmp_idx]->new_idx;
> > adjust_jmp_offset(insn->insn, new_jmp_idx) - insn->orig_idx;
> > }
>
> Hmm, this algo is kinds of the same this RFC, just we have organized "new_index"
> as "idx_map". And in this RFC, only new_idx of one original insn matters,
> no space is allocated for patched insns. (As mentioned, JIT blinding
It's not really about saving space. It's about having a mapping from
original index to a new one (in this case, through struct
bpf_patchable_insn *), which stays correct at all times, thus allowing
to not linearize between patching passes.
> requires the last insn inside patch buffer relocated to original jump
> offset, so there was a little special handling in the relocation loop in
> core layer linearization code)
>
> > The idea is that we want to support quick look-up by original
> > instruction index. That's what orig_idx_to_patchable_insn provides. On
> > the other hand, no existing instruction is ever referencing newly
> > patched instruction by its new offset, so with careful implementation,
> > you can transparently support all the cases, regardless if it's in
> > core layer or verifier layer (so, e.g., verifier layer patched
> > instructions now will be able to jump out of patched buffer, if
> > necessary, neat, right?).
> >
> > It is cleaner than everything we've discussed so far. Unless I missed
> > something critical (it's all quite convoluted, so I might have
> > forgotten some parts already). Let me know what you think.
>
> Let me digest a little bit and do some coding, then I will come back. Some
Sure, give it some thought and give it a go at coding, I bet overall
it will turn out more succinct and simpler. Please post an updated
version when you are done. Thanks!
> issues can only shown up during in-depth coding. I kind of feel handling
> aux reference in verifier layer is the part that will still introduce some
> un-clean code.
>
> <snip>
> >> If there is no dead insn elimination opt, then we could just adjust
> >> offsets. When there is insn deleting, I feel the logic becomes more
> >> complex. One subprog could be completely deleted or partially deleted, so
> >> I feel just recalculate the whole subprog info as a side-product is
> >> much simpler.
> >
> > What's the situation where entirety of subprog can be deleted?
>
> Suppose you have conditional jmp_imm, true path calls one subprog, false
> path calls the other. If insn walker later found it is also true, then the
> subprog at false path won't be marked as "seen", so it is entirely deleted.
>
> I actually thought it is in theory one subprog could be deleted entirely,
> so if we support insn deletion inside verifier, then range info like
> line_info/subprog_info needs to consider one range is deleted.
Seems like this is not a problem, according to Alexei. But in the
worst case, it's now simple to re-calculate all this, given that we
have this simple operation to get new insn idx by old insn idx.
>
> Thanks.
> Regards,
> Jiong
^ permalink raw reply
* Re: [PATCH bpf] selftests/bpf: make directory prerequisites order-only
From: Alexei Starovoitov @ 2019-07-16 17:49 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Ilya Leoshkevich, bpf, Network Development, gor, Heiko Carstens
In-Reply-To: <a3823fec-3816-9c38-bb2d-a8391766e64d@iogearbox.net>
On Mon, Jul 15, 2019 at 3:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/12/19 3:56 PM, Ilya Leoshkevich wrote:
> > When directories are used as prerequisites in Makefiles, they can cause
> > a lot of unnecessary rebuilds, because a directory is considered changed
> > whenever a file in this directory is added, removed or modified.
> >
> > If the only thing a target is interested in is the existence of the
> > directory it depends on, which is the case for selftests/bpf, this
> > directory should be specified as an order-only prerequisite: it would
> > still be created in case it does not exist, but it would not trigger a
> > rebuild of a target in case it's considered changed.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>
> Applied, thanks!
Hi Ilya,
this commit breaks map_tests.
To reproduce:
rm map_tests/tests.h
make
tests.h will not be regenerated.
Please provide a fix asap.
We cannot ship bpf tree with such failure.
^ permalink raw reply
* Re: [PATCH bpf] selftests/bpf: fix perf_buffer on s390
From: Andrii Nakryiko @ 2019-07-16 17:42 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, Networking, gor, heiko.carstens
In-Reply-To: <20190716125827.24413-1-iii@linux.ibm.com>
On Tue, Jul 16, 2019 at 5:59 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> perf_buffer test fails for exactly the same reason test_attach_probe
> used to fail: different nanosleep syscall kprobe name.
>
> Reuse the test_attach_probe fix.
>
> Fixes: ee5cf82ce04a ("selftests/bpf: test perf buffer API")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
Thanks for the fix!
Acked-by: Andrii Nakryiko <andriin@fb.com>
> .../testing/selftests/bpf/prog_tests/attach_probe.c | 12 ++----------
> tools/testing/selftests/bpf/prog_tests/perf_buffer.c | 8 +-------
> tools/testing/selftests/bpf/test_progs.h | 8 ++++++++
> 3 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index 47af4afc5013..5ecc267d98b0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -21,14 +21,6 @@ ssize_t get_base_addr() {
> return -EINVAL;
> }
>
> -#ifdef __x86_64__
> -#define SYS_KPROBE_NAME "__x64_sys_nanosleep"
> -#elif defined(__s390x__)
> -#define SYS_KPROBE_NAME "__s390x_sys_nanosleep"
> -#else
> -#define SYS_KPROBE_NAME "sys_nanosleep"
> -#endif
> -
> void test_attach_probe(void)
> {
> const char *kprobe_name = "kprobe/sys_nanosleep";
> @@ -86,7 +78,7 @@ void test_attach_probe(void)
>
> kprobe_link = bpf_program__attach_kprobe(kprobe_prog,
> false /* retprobe */,
> - SYS_KPROBE_NAME);
> + SYS_NANOSLEEP_KPROBE_NAME);
> if (CHECK(IS_ERR(kprobe_link), "attach_kprobe",
> "err %ld\n", PTR_ERR(kprobe_link))) {
> kprobe_link = NULL;
> @@ -94,7 +86,7 @@ void test_attach_probe(void)
> }
> kretprobe_link = bpf_program__attach_kprobe(kretprobe_prog,
> true /* retprobe */,
> - SYS_KPROBE_NAME);
> + SYS_NANOSLEEP_KPROBE_NAME);
> if (CHECK(IS_ERR(kretprobe_link), "attach_kretprobe",
> "err %ld\n", PTR_ERR(kretprobe_link))) {
> kretprobe_link = NULL;
> diff --git a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
> index 3f1ef95865ff..3003fddc0613 100644
> --- a/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
> +++ b/tools/testing/selftests/bpf/prog_tests/perf_buffer.c
> @@ -5,12 +5,6 @@
> #include <sys/socket.h>
> #include <test_progs.h>
>
> -#ifdef __x86_64__
> -#define SYS_KPROBE_NAME "__x64_sys_nanosleep"
> -#else
> -#define SYS_KPROBE_NAME "sys_nanosleep"
> -#endif
> -
> static void on_sample(void *ctx, int cpu, void *data, __u32 size)
> {
> int cpu_data = *(int *)data, duration = 0;
> @@ -56,7 +50,7 @@ void test_perf_buffer(void)
>
> /* attach kprobe */
> link = bpf_program__attach_kprobe(prog, false /* retprobe */,
> - SYS_KPROBE_NAME);
> + SYS_NANOSLEEP_KPROBE_NAME);
> if (CHECK(IS_ERR(link), "attach_kprobe", "err %ld\n", PTR_ERR(link)))
> goto out_close;
>
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index f095e1d4c657..49e0f7d85643 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -92,3 +92,11 @@ int compare_map_keys(int map1_fd, int map2_fd);
> int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len);
> int extract_build_id(char *build_id, size_t size);
> void *spin_lock_thread(void *arg);
> +
> +#ifdef __x86_64__
> +#define SYS_NANOSLEEP_KPROBE_NAME "__x64_sys_nanosleep"
> +#elif defined(__s390x__)
> +#define SYS_NANOSLEEP_KPROBE_NAME "__s390x_sys_nanosleep"
> +#else
> +#define SYS_NANOSLEEP_KPROBE_NAME "sys_nanosleep"
> +#endif
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix another GCC8 warning for strncpy
From: Alexei Starovoitov @ 2019-07-16 17:35 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, bpf,
Network Development, Andrii Nakryiko, Kernel Team,
Magnus Karlsson
In-Reply-To: <CAJ8uoz03xFA4TW7GNmLAw_A0wMjHUjYU2rG3pRWsEX-sAX8BFw@mail.gmail.com>
On Tue, Jul 16, 2019 at 10:31 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> On Tue, Jul 16, 2019 at 5:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Similar issue was fixed in cdfc7f888c2a ("libbpf: fix GCC8 warning for
> > strncpy") already. This one was missed. Fixing now.
>
> Thanks Andrii.
>
> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
Applied. Thanks
^ permalink raw reply
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
From: Stanislav Fomichev @ 2019-07-16 17:32 UTC (permalink / raw)
To: Martin Lau
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, Alexei Starovoitov,
Daniel Borkmann, David Miller, Kernel Team
In-Reply-To: <20190716054624.ea6sbbzn62grde2n@kafai-mbp>
On 07/16, Martin Lau wrote:
> On Tue, Jul 09, 2019 at 09:33:21AM -0700, Stanislav Fomichev wrote:
> > On 06/11, Martin KaFai Lau wrote:
> > > The cloned sk should not carry its parent-listener's sk_bpf_storage.
> > > This patch fixes it by setting it back to NULL.
> > Have you thought about some kind of inheritance for listener sockets'
> > storage? Suppose I have a situation where I write something
> > to listener's sk storage (directly or via recently added sockopts hooks)
> > and I want to inherit that state for a freshly established connection.
> >
> > I was looking into adding possibility to call bpf_get_listener_sock form
> > BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB callback to manually
> > copy some data form the listener socket, but I don't think
> > at this point there is any association between newly established
> > socket and the listener.
> Right, at that point, the child sk has no reference back
> to the listener's sk.
>
> After a quick look, the listener sk may not always be available
> also (e.g. the backlog processing case). Hence, adding
> the listener sk to the bpf running ctx is not obvious
> either.
>
> >
> > Thoughts/ideas?
> I think cloning the listener's bpf sk storage could be added
> to the existing sk cloning logic. It seems to be a more straight
> forward approach instead of figuring out the right place to call
> another bpf prog to clone it.
>
> Quick thoughts out of my head:
> 1. Default should be not-to-clone. Have a way (a map's flag?) to opt-in.
> 2. The listener's sk storage could be being modified while being cloned.
> One possibility is to check if the value has bpf_spin_lock.
> If there is, lock it before cloning.
Thanks for suggestion! An optional inherit/clone flag to
bpf_sk_storage_get seems like a good option. I'll try to play with it,
will probably get back with an rfc at some point.
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix another GCC8 warning for strncpy
From: Magnus Karlsson @ 2019-07-16 17:31 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, Network Development,
Andrii Nakryiko, kernel-team, Magnus Karlsson
In-Reply-To: <20190716035704.948081-1-andriin@fb.com>
On Tue, Jul 16, 2019 at 5:59 AM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Similar issue was fixed in cdfc7f888c2a ("libbpf: fix GCC8 warning for
> strncpy") already. This one was missed. Fixing now.
Thanks Andrii.
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/xsk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index b33740221b7e..5007b5d4fd2c 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -517,7 +517,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> err = -errno;
> goto out_socket;
> }
> - strncpy(xsk->ifname, ifname, IFNAMSIZ);
> + strncpy(xsk->ifname, ifname, IFNAMSIZ - 1);
> + xsk->ifname[IFNAMSIZ - 1] = '\0';
>
> err = xsk_set_xdp_socket_config(&xsk->config, usr_config);
> if (err)
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH -next] iwlwifi: dbg: work around clang bug by marking debug strings static
From: Nick Desaulniers @ 2019-07-16 17:28 UTC (permalink / raw)
To: Joe Perches, Kalle Valo
Cc: Arnd Bergmann, Nathan Chancellor, Johannes Berg,
Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless,
David S. Miller, Shahar S Matityahu, Sara Sharon, linux-wireless,
netdev, LKML, clang-built-linux
In-Reply-To: <b219cf41933b2f965572af515cf9d3119293bfba.camel@perches.com>
On Thu, Jul 11, 2019 at 7:15 PM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2019-07-11 at 17:17 -0700, Nick Desaulniers wrote:
> > Commit r353569 in prerelease Clang-9 is producing a linkage failure:
> >
> > ld: drivers/net/wireless/intel/iwlwifi/fw/dbg.o:
> > in function `_iwl_fw_dbg_apply_point':
> > dbg.c:(.text+0x827a): undefined reference to `__compiletime_assert_2387'
> >
> > when the following configs are enabled:
> > - CONFIG_IWLWIFI
> > - CONFIG_IWLMVM
> > - CONFIG_KASAN
> >
> > Work around the issue for now by marking the debug strings as `static`,
> > which they probably should be any ways.
> >
> > Link: https://bugs.llvm.org/show_bug.cgi?id=42580
> > Link: https://github.com/ClangBuiltLinux/linux/issues/580
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> > Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > drivers/net/wireless/intel/iwlwifi/fw/dbg.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > index e411ac98290d..f8c90ea4e9b4 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> > @@ -2438,7 +2438,7 @@ static void iwl_fw_dbg_info_apply(struct iwl_fw_runtime *fwrt,
> > {
> > u32 img_name_len = le32_to_cpu(dbg_info->img_name_len);
> > u32 dbg_cfg_name_len = le32_to_cpu(dbg_info->dbg_cfg_name_len);
> > - const char err_str[] =
> > + static const char err_str[] =
> > "WRT: ext=%d. Invalid %s name length %d, expected %d\n";
>
> Better still would be to use the format string directly
> in both locations instead of trying to deduplicate it
> via storing it into a separate pointer.
>
> Let the compiler/linker consolidate the format.
> It's smaller object code, allows format/argument verification,
> and is simpler for humans to understand.
Whichever Kalle prefers, I just want my CI green again.
>
> ---
> diff --git a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> index e411ac98290d..25e6712932b8 100644
> --- a/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> +++ b/drivers/net/wireless/intel/iwlwifi/fw/dbg.c
> @@ -2438,17 +2438,17 @@ static void iwl_fw_dbg_info_apply(struct iwl_fw_runtime *fwrt,
> {
> u32 img_name_len = le32_to_cpu(dbg_info->img_name_len);
> u32 dbg_cfg_name_len = le32_to_cpu(dbg_info->dbg_cfg_name_len);
> - const char err_str[] =
> - "WRT: ext=%d. Invalid %s name length %d, expected %d\n";
>
> if (img_name_len != IWL_FW_INI_MAX_IMG_NAME_LEN) {
> - IWL_WARN(fwrt, err_str, ext, "image", img_name_len,
> + IWL_WARN(fwrt, "WRT: ext=%d. Invalid %s name length %d, expected %d\n",
> + ext, "image", img_name_len,
> IWL_FW_INI_MAX_IMG_NAME_LEN);
> return;
> }
>
> if (dbg_cfg_name_len != IWL_FW_INI_MAX_DBG_CFG_NAME_LEN) {
> - IWL_WARN(fwrt, err_str, ext, "debug cfg", dbg_cfg_name_len,
> + IWL_WARN(fwrt, "WRT: ext=%d. Invalid %s name length %d, expected %d\n",
> + ext, "debug cfg", dbg_cfg_name_len,
> IWL_FW_INI_MAX_DBG_CFG_NAME_LEN);
> return;
> }
> @@ -2775,8 +2775,6 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
> struct iwl_ucode_tlv *tlv = iter;
> void *ini_tlv = (void *)tlv->data;
> u32 type = le32_to_cpu(tlv->type);
> - const char invalid_ap_str[] =
> - "WRT: ext=%d. Invalid apply point %d for %s\n";
>
> switch (type) {
> case IWL_UCODE_TLV_TYPE_DEBUG_INFO:
> @@ -2786,8 +2784,8 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
> struct iwl_fw_ini_allocation_data *buf_alloc = ini_tlv;
>
> if (pnt != IWL_FW_INI_APPLY_EARLY) {
> - IWL_ERR(fwrt, invalid_ap_str, ext, pnt,
> - "buffer allocation");
> + IWL_ERR(fwrt, "WRT: ext=%d. Invalid apply point %d for %s\n",
> + ext, pnt, "buffer allocation");
> goto next;
> }
>
> @@ -2797,8 +2795,8 @@ static void _iwl_fw_dbg_apply_point(struct iwl_fw_runtime *fwrt,
> }
> case IWL_UCODE_TLV_TYPE_HCMD:
> if (pnt < IWL_FW_INI_APPLY_AFTER_ALIVE) {
> - IWL_ERR(fwrt, invalid_ap_str, ext, pnt,
> - "host command");
> + IWL_ERR(fwrt, "WRT: ext=%d. Invalid apply point %d for %s\n",
> + ext, pnt, "host command");
> goto next;
> }
> iwl_fw_dbg_send_hcmd(fwrt, tlv, ext);
>
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()
From: Anton Protopopov @ 2019-07-16 17:14 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, Networking, bpf, open list, Andrii Nakryiko
In-Reply-To: <CAEf4BzYaiH_GhSJJjkGv4dGF7CbBrusTyShPP9DXvXjCLcmK+w@mail.gmail.com>
вт, 9 июл. 2019 г. в 13:40, Andrii Nakryiko <andrii.nakryiko@gmail.com>:
>
> On Mon, Jul 8, 2019 at 1:37 PM Anton Protopopov
> <a.s.protopopov@gmail.com> wrote:
> >
> > пн, 8 июл. 2019 г. в 13:54, Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> > >
> > > On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > >
> > > > On 07/05/2019 10:44 PM, Anton Protopopov wrote:
> > > > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in
> > > > > an object by maps pinned to a directory provided in the path argument. Namely,
> > > > > each map M in the object will be replaced by a map pinned to path/M.name.
> > > > >
> > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com>
> > > > > ---
> > > > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > tools/lib/bpf/libbpf.h | 2 ++
> > > > > tools/lib/bpf/libbpf.map | 1 +
> > > > > 3 files changed, 37 insertions(+)
> > > > >
> > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > > index 4907997289e9..84c9e8f7bfd3 100644
> > > > > --- a/tools/lib/bpf/libbpf.c
> > > > > +++ b/tools/lib/bpf/libbpf.c
> > > > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path)
> > >
> > > As is, bpf_object__reuse_maps() can be easily implemented by user
> > > applications, as it's only using public libbpf APIs, so I'm not 100%
> > > sure we need to add method like that to libbpf.
> >
> > The bpf_object__reuse_maps() can definitely be implemented by user
> > applications, however, to use it a user also needs to re-implement the
> > bpf_prog_load_xattr funciton, so it seemed to me that adding this
> > functionality to the library is a better way.
>
> I'm still not convinced. Looking at bpf_prog_load_xattr, I think some
> of what it's doing should be part of bpf_object__object_xattr anyway
> (all the expected type setting for programs).
>
> Besides that, there isn't much more than just bpf_object__open and
> bpf_object__load, to be honest. By doing open and load explicitly,
> user gets an opportunity to do whatever adjustment they need: reuse
> maps, adjust map sizes, etc. So I think we should improve
> bpf_object__open to "guess" program attach types and add map
> definition flags to allow reuse declaratively.
>
>
> >
> > >
> > > > > +{
> > > > > + struct bpf_map *map;
> > > > > +
> > > > > + if (!obj)
> > > > > + return -ENOENT;
> > > > > +
> > > > > + if (!path)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + bpf_object__for_each_map(map, obj) {
> > > > > + int len, err;
> > > > > + int pinned_map_fd;
> > > > > + char buf[PATH_MAX];
> > > >
> > > > We'd need to skip the case of bpf_map__is_internal(map) since they are always
> > > > recreated for the given object.
> > > >
> > > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
> > > > > + if (len < 0) {
> > > > > + return -EINVAL;
> > > > > + } else if (len >= PATH_MAX) {
> > > > > + return -ENAMETOOLONG;
> > > > > + }
> > > > > +
> > > > > + pinned_map_fd = bpf_obj_get(buf);
> > > > > + if (pinned_map_fd < 0)
> > > > > + return pinned_map_fd;
> > > >
> > > > Should we rather have a new map definition attribute that tells to reuse
> > > > the map if it's pinned in bpf fs, and if not, we create it and later on
> > > > pin it? This is what iproute2 is doing and which we're making use of heavily.
> > >
> > > I'd like something like that as well. This would play nicely with
> > > recently added BTF-defined maps as well.
> > >
> > > I think it should be not just pin/don't pin flag, but rather pinning
> > > strategy, to accommodate various typical strategies of handling maps
> > > that are already pinned. So something like this:
> > >
> > > 1. BPF_PIN_NOTHING - default, don't pin;
> > > 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail;
> > > 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be
> > > exact state of object's map;
> > > 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this
> > > is how Cilium is pinning PROG_ARRAY maps, if I understand correctly);
> > > 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values.
> > >
> > > This list is only for illustrative purposes, ideally people that have
> > > a lot of experience using pinning for real-world use cases would chime
> > > in on what strategies are useful and make sense.
> >
> > My case was simply to reuse existing maps when reloading a program.
> > Does it make sense for you to add only the simplest cases of listed above?
>
> Of course, it's enum, so we can start with few clearly useful ones and
> then expand more if we ever have a need. But I think we still need a
> bit wider discussion and let people who use pinning to chime in.
>
> >
> > Also, libbpf doesn't use standard naming conventions for pinning maps.
>
> We talked about this in another thread related to BTF-defined maps. I
> think the way to go with this is to actually define a default pinning
> root path, but allow to override it on bpf_object__open, if user needs
> a different one.
>
> > Does it make sense to provide a list of already open maps to the
> > bpf_prog_load_xattr function as an attribute? In this case a user
> > can execute his own policy on pinning, but still will have an option
> > to reuse, reset, and merge maps.
>
> As explained above, I don't think there isn't much added value in
> bpf_prog_load, so I'd advise to just switch to explicit
> bpf_object__open + bpf_object__load and get maximum control and
> flexibility.
Thanks for your comments. I can see now that using
bpf_object__open/bpf_object__load makes better sense.
>
> >
> > >
> > > > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps
> > > > too limiting for a generic API as new version of an object file may contain
> > > > new maps which are not yet present in bpf fs at that point.
> > > >
> > > > > + err = bpf_map__reuse_fd(map, pinned_map_fd);
> > > > > + if (err)
> > > > > + return err;
> > > > > + }
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
> > > > > {
> > > > > struct bpf_program *prog;
> > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > > index d639f47e3110..7fe465a1be76 100644
> > > > > --- a/tools/lib/bpf/libbpf.h
> > > > > +++ b/tools/lib/bpf/libbpf.h
> > > > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
> > > > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
> > > > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
> > > > > const char *path);
> > > > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj,
> > > > > + const char *path);
> > > > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
> > > > > const char *path);
> > > > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
> > > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > > > index 2c6d835620d2..66a30be6696c 100644
> > > > > --- a/tools/lib/bpf/libbpf.map
> > > > > +++ b/tools/lib/bpf/libbpf.map
> > > > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 {
> > > > > btf_dump__new;
> > > > > btf__parse_elf;
> > > > > bpf_object__load_xattr;
> > > > > + bpf_object__reuse_maps;
> > > > > libbpf_num_possible_cpus;
> > > > > } LIBBPF_0.0.3;
> > > > >
> > > >
^ permalink raw reply
* Re: [PATCH net v2] skbuff: fix compilation warnings in skb_dump()
From: Nathan Chancellor @ 2019-07-16 16:51 UTC (permalink / raw)
To: Qian Cai; +Cc: davem, willemb, joe, clang-built-linux, netdev, linux-kernel
In-Reply-To: <1563291785-6545-1-git-send-email-cai@lca.pw>
On Tue, Jul 16, 2019 at 11:43:05AM -0400, Qian Cai wrote:
> The commit 6413139dfc64 ("skbuff: increase verbosity when dumping skb
> data") introduced a few compilation warnings.
>
> net/core/skbuff.c:766:32: warning: format specifies type 'unsigned
> short' but the argument has type 'unsigned int' [-Wformat]
> level, sk->sk_family, sk->sk_type,
> sk->sk_protocol);
> ^~~~~~~~~~~
> net/core/skbuff.c:766:45: warning: format specifies type 'unsigned
> short' but the argument has type 'unsigned int' [-Wformat]
> level, sk->sk_family, sk->sk_type,
> sk->sk_protocol);
> ^~~~~~~~~~~~~~~
>
> Fix them by using the proper types.
>
> Fixes: 6413139dfc64 ("skbuff: increase verbosity when dumping skb data")
> Signed-off-by: Qian Cai <cai@lca.pw>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
^ permalink raw reply
* Re: IPv6 L2TP issues related to 93531c67
From: David Ahern @ 2019-07-16 16:46 UTC (permalink / raw)
To: Paul Donohue; +Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
In-Reply-To: <20190716135646.GE2622@TopQuark.net>
On 7/16/19 7:56 AM, Paul Donohue wrote:
> After establishing an IPsec tunnel to carry the L2TP traffic, the first L2TP packet through the IPsec tunnel permanently breaks the associated L2TP tunnel. Tearing down the IPsec tunnel does not restore functionality of the L2TP tunnel - I have to tear down and re-create the L2TP tunnel before it will work again. In my real-world use case, I have two L2TP tunnels running over the same IPsec tunnel, and the first L2TP tunnel to send a packet after IPsec is established gets permanently broken, while the other L2TP tunnel works fine.
>
> I've attached a modified version of the script which demonstrates this issue.
Thanks. I will take a look at get back to you.
^ permalink raw reply
* [PATCH 0/2] mmc: core: Fix Marvell WiFi reset by adding SDIO API to replug card
From: Douglas Anderson @ 2019-07-16 16:42 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter
Cc: Ganapathi Bhat, linux-wireless, Brian Norris, Amitkumar Karwar,
linux-rockchip, Wolfram Sang, Nishant Sarmukadam, netdev,
Avri Altman, linux-mmc, davem, Xinming Hu, Douglas Anderson,
Jiong Wu, Ritesh Harjani, linux-kernel, Thomas Gleixner,
Greg Kroah-Hartman, Niklas Söderlund
As talked about in the thread at:
http://lkml.kernel.org/r/CAD=FV=X7P2F1k_zwHc0mbtfk55-rucTz_GoDH=PL6zWqKYcpuw@mail.gmail.com
...when the Marvell WiFi card tries to reset itself it kills
Bluetooth. It was observed that we could re-init the card properly by
unbinding / rebinding the host controller. It was also observed that
in the downstream Chrome OS codebase the solution used was
mmc_remove_host() / mmc_add_host(), which is similar to the solution
in this series.
So far I've only done testing of this series using the reset test
source that can be simulated via sysfs. Specifically I ran this test:
for i in $(seq 1000); do
echo "LOOP $i --------"
echo 1 > /sys/kernel/debug/mwifiex/mlan0/reset
while true; do
if ! ping -w15 -c1 "${GW}" >/dev/null 2>&1; then
fail=$(( fail + 1 ))
echo "Fail WiFi ${fail}"
if [[ ${fail} == 3 ]]; then
exit 1
fi
else
fail=0
break
fi
done
hciconfig hci0 down
sleep 1
if ! hciconfig hci0 up; then
echo "Fail BT"
exit 1
fi
done
I ran this several times and got several hundred iterations each
before a failure. When I saw failures:
* Once I saw a "Fail BT"; manually resetting the card again fixed it.
I didn't give it time to see if it would have detected this
automatically.
* Once I saw the ping fail because (for some reason) my device only
got an IPv6 address from my router and the IPv4 ping failed. I
changed my script to use 'ping6' to see if that would help.
* Once I saw the ping fail because the higher level network stack
("shill" in my case) seemed to crash. A few minutes later the
system recovered itself automatically. https://crbug.com/984593 if
you want more details.
* Sometimes while I was testing I saw "Fail WiFi 1" indicating a
transitory failure. Usually this was an association failure, but in
one case I saw the device do "Firmware wakeup failed" after I
triggered the reset. This caused the driver to trigger a re-reset
of itself which eventually recovered things. This was good because
it was an actual test of the normal reset flow (not the one
triggered via sysfs).
Douglas Anderson (2):
mmc: core: Add sdio_trigger_replug() API
mwifiex: Make use of the new sdio_trigger_replug() API to reset
drivers/mmc/core/core.c | 28 +++++++++++++++++++--
drivers/mmc/core/sdio_io.c | 20 +++++++++++++++
drivers/net/wireless/marvell/mwifiex/sdio.c | 14 +++--------
include/linux/mmc/host.h | 15 ++++++++++-
include/linux/mmc/sdio_func.h | 2 ++
5 files changed, 65 insertions(+), 14 deletions(-)
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply
* [PATCH 1/2] mmc: core: Add sdio_trigger_replug() API
From: Douglas Anderson @ 2019-07-16 16:42 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter
Cc: Ganapathi Bhat, linux-wireless, Brian Norris, Amitkumar Karwar,
linux-rockchip, Wolfram Sang, Nishant Sarmukadam, netdev,
Avri Altman, linux-mmc, davem, Xinming Hu, Douglas Anderson,
Jiong Wu, Ritesh Harjani, linux-kernel, Thomas Gleixner,
Greg Kroah-Hartman, Niklas Söderlund
In-Reply-To: <20190716164209.62320-1-dianders@chromium.org>
When using Marvell WiFi SDIO cards, it is not uncommon for Linux WiFi
driver to fully lose the communication channel to the firmware running
on the card. Presumably the firmware on the card has a bug or two in
it and occasionally crashes.
The Marvell WiFi driver attempts to recover from this problem.
Specifically the driver has the function mwifiex_sdio_card_reset()
which is called when communcation problems are found. That function
attempts to reset the state of things by utilizing the mmc_hw_reset()
function.
The current solution is a bit complex because the Marvell WiFi driver
needs to manually deinit and reinit the WiFi driver around the reset
call. This means it's going through a bunch of code paths that aren't
normally tested. However, complexity isn't our only problem. The
other (bigger) problem is that Marvell WiFi cards are often combo
WiFi/Bluetooth cards and Bluetooth runs on a second SDIO func. While
the WiFi driver knows that it should re-init its own state around the
mmc_hw_reset() call there is no good way to inform the Bluetooth
driver. That means that in Linux today when you reset the Marvell
WiFi driver you lose all Bluetooth communication. Doh!
One way to fix the above problems is to leverage a more standard way
to reset the Marvell WiFi card where we go through the same code paths
as card unplug and the card plug. In this patch we introduce a new
API call for doing just that: sdio_trigger_replug(). This API call
will trigger an unplug of the SDIO card followed by a plug of the
card. As part of this the card will be nicely reset.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/mmc/core/core.c | 28 ++++++++++++++++++++++++++--
drivers/mmc/core/sdio_io.c | 20 ++++++++++++++++++++
include/linux/mmc/host.h | 15 ++++++++++++++-
include/linux/mmc/sdio_func.h | 2 ++
4 files changed, 62 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 9020cb2490f7..48a7d23aed26 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2164,6 +2164,12 @@ int mmc_sw_reset(struct mmc_host *host)
}
EXPORT_SYMBOL(mmc_sw_reset);
+void mmc_trigger_replug(struct mmc_host *host)
+{
+ host->trigger_replug_state = MMC_REPLUG_STATE_UNPLUG;
+ _mmc_detect_change(host, 0, false);
+}
+
static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
{
host->f_init = freq;
@@ -2217,6 +2223,11 @@ int _mmc_detect_card_removed(struct mmc_host *host)
if (!host->card || mmc_card_removed(host->card))
return 1;
+ if (host->trigger_replug_state == MMC_REPLUG_STATE_UNPLUG) {
+ mmc_card_set_removed(host->card);
+ return 1;
+ }
+
ret = host->bus_ops->alive(host);
/*
@@ -2329,8 +2340,21 @@ void mmc_rescan(struct work_struct *work)
mmc_bus_put(host);
mmc_claim_host(host);
- if (mmc_card_is_removable(host) && host->ops->get_cd &&
- host->ops->get_cd(host) == 0) {
+
+ /*
+ * Move through the state machine if we're triggering an unplug
+ * followed by a re-plug.
+ */
+ if (host->trigger_replug_state == MMC_REPLUG_STATE_UNPLUG) {
+ host->trigger_replug_state = MMC_REPLUG_STATE_PLUG;
+ _mmc_detect_change(host, 0, false);
+ } else if (host->trigger_replug_state == MMC_REPLUG_STATE_PLUG) {
+ host->trigger_replug_state = MMC_REPLUG_STATE_NONE;
+ }
+
+ if (host->trigger_replug_state == MMC_REPLUG_STATE_PLUG ||
+ (mmc_card_is_removable(host) && host->ops->get_cd &&
+ host->ops->get_cd(host) == 0)) {
mmc_power_off(host);
mmc_release_host(host);
goto out;
diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 2ba00acf64e6..1c5c2a3ebe5e 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -811,3 +811,23 @@ void sdio_retune_release(struct sdio_func *func)
mmc_retune_release(func->card->host);
}
EXPORT_SYMBOL_GPL(sdio_retune_release);
+
+/**
+ * sdio_trigger_replug - trigger an "unplug" + "plug" of the card
+ * @func: SDIO function attached to host
+ *
+ * When you call this function we will schedule events that will
+ * make it look like the card contining the given SDIO func was
+ * unplugged and then re-plugged-in. This is as close as possible
+ * to a full reset of the card that can be achieved.
+ *
+ * NOTE: routnine will temporarily make the card look as if it is
+ * removable even if it is marked non-removable.
+ *
+ * This function should be called while the host is claimed.
+ */
+void sdio_trigger_replug(struct sdio_func *func)
+{
+ mmc_trigger_replug(func->card->host);
+}
+EXPORT_SYMBOL(sdio_trigger_replug);
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index a9b12322c775..e0d41a1bcf17 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -410,6 +410,12 @@ struct mmc_host {
bool trigger_card_event; /* card_event necessary */
+ /* state machine for triggering unplug/replug */
+#define MMC_REPLUG_STATE_NONE 0 /* not doing unplug/replug */
+#define MMC_REPLUG_STATE_UNPLUG 1 /* do unplug next */
+#define MMC_REPLUG_STATE_PLUG 2 /* do plug next */
+ u8 trigger_replug_state;
+
struct mmc_card *card; /* device attached to this host */
wait_queue_head_t wq;
@@ -530,7 +536,12 @@ int mmc_regulator_get_supply(struct mmc_host *mmc);
static inline int mmc_card_is_removable(struct mmc_host *host)
{
- return !(host->caps & MMC_CAP_NONREMOVABLE);
+ /*
+ * A non-removable card briefly looks removable if code has forced
+ * a re-plug of the card.
+ */
+ return host->trigger_replug_state != MMC_REPLUG_STATE_NONE ||
+ !(host->caps & MMC_CAP_NONREMOVABLE);
}
static inline int mmc_card_keep_power(struct mmc_host *host)
@@ -583,4 +594,6 @@ static inline enum dma_data_direction mmc_get_dma_dir(struct mmc_data *data)
int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
int mmc_abort_tuning(struct mmc_host *host, u32 opcode);
+void mmc_trigger_replug(struct mmc_host *host);
+
#endif /* LINUX_MMC_HOST_H */
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 5a177f7a83c3..0d6c73768ae3 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -173,4 +173,6 @@ extern void sdio_retune_crc_enable(struct sdio_func *func);
extern void sdio_retune_hold_now(struct sdio_func *func);
extern void sdio_retune_release(struct sdio_func *func);
+extern void sdio_trigger_replug(struct sdio_func *func);
+
#endif /* LINUX_MMC_SDIO_FUNC_H */
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH 2/2] mwifiex: Make use of the new sdio_trigger_replug() API to reset
From: Douglas Anderson @ 2019-07-16 16:42 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter
Cc: Ganapathi Bhat, linux-wireless, Brian Norris, Amitkumar Karwar,
linux-rockchip, Wolfram Sang, Nishant Sarmukadam, netdev,
Avri Altman, linux-mmc, davem, Xinming Hu, Douglas Anderson,
linux-kernel
In-Reply-To: <20190716164209.62320-1-dianders@chromium.org>
As described in the patch ("mmc: core: Add sdio_trigger_replug()
API"), the current mwifiex_sdio_card_reset() is broken in the cases
where we're running Bluetooth on a second SDIO func on the same card
as WiFi. The problem goes away if we just use the
sdio_trigger_replug() API call.
NOTE: Even though with this new solution there is less of a reason to
do our work from a workqueue (the unplug / plug mechanism we're using
is possible for a human to perform at any time so the stack is
supposed to handle it without it needing to be called from a special
context), we still need a workqueue because the Marvell reset function
could called from a context where sleeping is invalid and thus we
can't claim the host. One example is Marvell's wakeup_timer_fn().
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 24c041dad9f6..f77ad2615f08 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2218,14 +2218,6 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter)
{
struct sdio_mmc_card *card = adapter->card;
struct sdio_func *func = card->func;
- int ret;
-
- mwifiex_shutdown_sw(adapter);
-
- /* power cycle the adapter */
- sdio_claim_host(func);
- mmc_hw_reset(func->card->host);
- sdio_release_host(func);
/* Previous save_adapter won't be valid after this. We will cancel
* pending work requests.
@@ -2233,9 +2225,9 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter)
clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
- ret = mwifiex_reinit_sw(adapter);
- if (ret)
- dev_err(&func->dev, "reinit failed: %d\n", ret);
+ sdio_claim_host(func);
+ sdio_trigger_replug(func);
+ sdio_release_host(func);
}
/* This function read/write firmware */
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* Re: [RFC PATCH 0/5] PTP: add support for Intel's TGPIO controller
From: Richard Cochran @ 2019-07-16 16:41 UTC (permalink / raw)
To: Felipe Balbi
Cc: netdev, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, x86, linux-kernel, Christopher S . Hall
In-Reply-To: <20190716072038.8408-1-felipe.balbi@linux.intel.com>
On Tue, Jul 16, 2019 at 10:20:33AM +0300, Felipe Balbi wrote:
> TGPIO is a new IP which allows for time synchronization between systems
> without any other means of synchronization such as PTP or NTP. The
> driver is implemented as part of the PTP framework since its features
> covered most of what this controller can do.
Can you provide some background on this new HW? Is the interface
copper wires between chips? Or is it perhaps coax between hosts?
Thanks,
Richard
^ permalink raw reply
* Re: [RFC PATCH 4/5] PTP: Add flag for non-periodic output
From: Richard Cochran @ 2019-07-16 16:39 UTC (permalink / raw)
To: Felipe Balbi
Cc: netdev, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, x86, linux-kernel, Christopher S . Hall
In-Reply-To: <20190716072038.8408-5-felipe.balbi@linux.intel.com>
On Tue, Jul 16, 2019 at 10:20:37AM +0300, Felipe Balbi wrote:
> When this new flag is set, we can use single-shot output.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> include/uapi/linux/ptp_clock.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h
> index 674db7de64f3..439cbdfc3d9b 100644
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -67,7 +67,9 @@ struct ptp_perout_request {
> struct ptp_clock_time start; /* Absolute start time. */
> struct ptp_clock_time period; /* Desired period, zero means disable. */
> unsigned int index; /* Which channel to configure. */
> - unsigned int flags; /* Reserved for future use. */
> +
> +#define PTP_PEROUT_ONE_SHOT BIT(0)
> + unsigned int flags; /* Bit 0 -> oneshot output. */
> unsigned int rsv[4]; /* Reserved for future use. */
Unfortunately, the code never checked that .flags and .rsv are zero,
and so the de-facto ABI makes extending these fields impossible. That
was my mistake from the beginning.
In order to actually support extensions, you will first have to
introduce a new ioctl.
Sorry,
Richard
> };
>
> --
> 2.22.0
>
^ permalink raw reply
* Re: [PATCH bpf v2] selftests/bpf: skip nmi test when perf hw events are disabled
From: Alexei Starovoitov @ 2019-07-16 16:27 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Ilya Leoshkevich, bpf, Networking, gor, heiko.carstens, Y Song
In-Reply-To: <CAEf4Bzaf2Ys6H4h0rk6z+QhP-anonz=MBej5CaShXKL453MB4A@mail.gmail.com>
On Tue, Jul 16, 2019 at 07:57:04AM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 16, 2019 at 3:56 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >
> > Some setups (e.g. virtual machines) might run with hardware perf events
> > disabled. If this is the case, skip the test_send_signal_nmi test.
> >
> > Add a separate test involving a software perf event. This allows testing
> > the perf event path regardless of hardware perf event support.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
>
> LGTM!
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Applied, Thanks
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-07-16 16:26 UTC (permalink / raw)
To: Paul Moore
Cc: Tycho Andersen, nhorman, linux-api, containers, LKML, dhowells,
Linux-Audit Mailing List, netfilter-devel, ebiederm, simo, netdev,
linux-fsdevel, Eric Paris, Serge E. Hallyn
In-Reply-To: <CAHC9VhTaLqCo8rmAaySJQB+Pf-580=3mvX1rPmtEeb9o5Uy9Qg@mail.gmail.com>
On 2019-07-16 12:08, Paul Moore wrote:
> On Tue, Jul 16, 2019 at 11:37 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2019-07-15 17:09, Paul Moore wrote:
> > > On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2019-05-30 19:26, Paul Moore wrote:
> > >
> > > ...
> > >
> > > > > I like the creativity, but I worry that at some point these
> > > > > limitations are going to be raised (limits have a funny way of doing
> > > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > > want to be able to quickly do an audit container ID comparison and
> > > > > we're going to pay a penalty for these larger values (we'll need this
> > > > > when we add multiple auditd support and the requisite record routing).
> > > > >
> > > > > Thinking about this makes me also realize we probably need to think a
> > > > > bit longer about audit container ID conflicts between orchestrators.
> > > > > Right now we just take the value that is given to us by the
> > > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > > to work without some form of cooperation in userspace (I think we have
> > > > > to assume the orchestrators will not talk to each other) we likely
> > > > > need to have some way to block reuse of an audit container ID. We
> > > > > would either need to prevent the orchestrator from explicitly setting
> > > > > an audit container ID to a currently in use value, or instead generate
> > > > > the audit container ID in the kernel upon an event triggered by the
> > > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > > start looking at the idr code, I think we will need to make use of it.
> > > >
> > > > To address this, I'd suggest that it is enforced to only allow the
> > > > setting of descendants and to maintain a master list of audit container
> > > > identifiers (with a hash table if necessary later) that includes the
> > > > container owner.
> > >
> > > We're discussing the audit container ID management policy elsewhere in
> > > this thread so I won't comment on that here, but I did want to say
> > > that we will likely need something better than a simple list of audit
> > > container IDs from the start. It's common for systems to have
> > > thousands of containers now (or multiple thousands), which tells me
> > > that a list is a poor choice. You mentioned a hash table, so I would
> > > suggest starting with that over the list for the initial patchset.
> >
> > I saw that as an internal incremental improvement that did not affect
> > the API, so I wanted to keep things a bit simpler (as you've requested
> > in the past) to get this going, and add that enhancement later.
>
> In general a simple approach is a good way to start when the
> problem/use-case is not very well understood; in other words, don't
> spend a lot of time/effort optimizing something you don't yet
> understand. In this case we know that people want to deploy a *lot*
> of containers on a single system so we should design the data
> structures appropriately. A list is simply not a good fit here, I
> believe/hope you know that too.
Yes, I knew that, which is why I alluded to a hash table...
> > I'll start working on it now. The hash table would simply point to
> > lists anyways unless you can recommend a better approach.
>
> I assume when you say "point to lists" you are talking about using
> lists for the hash buckets? If so, yes that should be fine at this
> point. In general if the per-bucket lists become a bottleneck we can
> look at the size of the table (or make it tunable) or even use a
> different approach entirely. Ultimately the data store is an
> implementation detail private to the audit subsystem in the kernel so
> we should be able to change it as necessary without breaking anything.
Yes, this is what I had in mind. It would be tunable either by a macro
or a config option, so the exact value isn't a critical implementation
detail that can be easily tuned as we gain experience with it. And yes,
the intent was that it was a non-user-perceivable implementation choice
other than performace metrics.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH bpf v3] selftests/bpf: fix "alu with different scalars 1" on s390
From: Alexei Starovoitov @ 2019-07-16 16:23 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev, gor, heiko.carstens, daniel, ys114321
In-Reply-To: <20190716105353.21704-1-iii@linux.ibm.com>
On Tue, Jul 16, 2019 at 12:53:53PM +0200, Ilya Leoshkevich wrote:
> BPF_LDX_MEM is used to load the least significant byte of the retrieved
> test_val.index, however, on big-endian machines it ends up retrieving
> the most significant byte.
>
> Change the test to load the whole int in order to make it
> endianness-independent.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Applied, Thanks
^ permalink raw reply
* Re: [RFC bpf-next 0/8] bpf: accelerate insn patching speed
From: Alexei Starovoitov @ 2019-07-16 16:17 UTC (permalink / raw)
To: Jiong Wang
Cc: Andrii Nakryiko, Daniel Borkmann, Edward Cree, Naveen N. Rao,
Andrii Nakryiko, Jakub Kicinski, bpf, Networking, oss-drivers,
Yonghong Song
In-Reply-To: <87wogitlbi.fsf@netronome.com>
On Tue, Jul 16, 2019 at 09:50:25AM +0100, Jiong Wang wrote:
>
> Let me digest a little bit and do some coding, then I will come back. Some
> issues can only shown up during in-depth coding. I kind of feel handling
> aux reference in verifier layer is the part that will still introduce some
> un-clean code.
I'm still internalizing this discussion. Only want to point out
that I think it's better to have simpler algorithm that consumes more
memory and slower than more complex algorithm that is more cpu/memory efficient.
Here we're aiming at 10x improvement anyway, so extra cpu and memory
here and there are good trade-off to make.
> >> If there is no dead insn elimination opt, then we could just adjust
> >> offsets. When there is insn deleting, I feel the logic becomes more
> >> complex. One subprog could be completely deleted or partially deleted, so
> >> I feel just recalculate the whole subprog info as a side-product is
> >> much simpler.
> >
> > What's the situation where entirety of subprog can be deleted?
>
> Suppose you have conditional jmp_imm, true path calls one subprog, false
> path calls the other. If insn walker later found it is also true, then the
> subprog at false path won't be marked as "seen", so it is entirely deleted.
>
> I actually thought it is in theory one subprog could be deleted entirely,
> so if we support insn deletion inside verifier, then range info like
> line_info/subprog_info needs to consider one range is deleted.
I don't think dead code elim can remove subprogs.
cfg check rejects code with dead progs.
I don't think we have a test for such 'dead prog only due to verifier walk'
situation. I wonder what happens :)
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-07-16 16:08 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Tycho Andersen, Serge E. Hallyn, containers, linux-api,
Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, sgrubb, omosnace, dhowells, simo, Eric Paris,
ebiederm, nhorman
In-Reply-To: <20190716153705.xx7dwrhliny5amut@madcap2.tricolour.ca>
On Tue, Jul 16, 2019 at 11:37 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-07-15 17:09, Paul Moore wrote:
> > On Mon, Jul 8, 2019 at 2:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2019-05-30 19:26, Paul Moore wrote:
> >
> > ...
> >
> > > > I like the creativity, but I worry that at some point these
> > > > limitations are going to be raised (limits have a funny way of doing
> > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > want to be able to quickly do an audit container ID comparison and
> > > > we're going to pay a penalty for these larger values (we'll need this
> > > > when we add multiple auditd support and the requisite record routing).
> > > >
> > > > Thinking about this makes me also realize we probably need to think a
> > > > bit longer about audit container ID conflicts between orchestrators.
> > > > Right now we just take the value that is given to us by the
> > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > to work without some form of cooperation in userspace (I think we have
> > > > to assume the orchestrators will not talk to each other) we likely
> > > > need to have some way to block reuse of an audit container ID. We
> > > > would either need to prevent the orchestrator from explicitly setting
> > > > an audit container ID to a currently in use value, or instead generate
> > > > the audit container ID in the kernel upon an event triggered by the
> > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > start looking at the idr code, I think we will need to make use of it.
> > >
> > > To address this, I'd suggest that it is enforced to only allow the
> > > setting of descendants and to maintain a master list of audit container
> > > identifiers (with a hash table if necessary later) that includes the
> > > container owner.
> >
> > We're discussing the audit container ID management policy elsewhere in
> > this thread so I won't comment on that here, but I did want to say
> > that we will likely need something better than a simple list of audit
> > container IDs from the start. It's common for systems to have
> > thousands of containers now (or multiple thousands), which tells me
> > that a list is a poor choice. You mentioned a hash table, so I would
> > suggest starting with that over the list for the initial patchset.
>
> I saw that as an internal incremental improvement that did not affect
> the API, so I wanted to keep things a bit simpler (as you've requested
> in the past) to get this going, and add that enhancement later.
In general a simple approach is a good way to start when the
problem/use-case is not very well understood; in other words, don't
spend a lot of time/effort optimizing something you don't yet
understand. In this case we know that people want to deploy a *lot*
of containers on a single system so we should design the data
structures appropriately. A list is simply not a good fit here, I
believe/hope you know that too.
> I'll start working on it now. The hash table would simply point to
> lists anyways unless you can recommend a better approach.
I assume when you say "point to lists" you are talking about using
lists for the hash buckets? If so, yes that should be fine at this
point. In general if the per-bucket lists become a bottleneck we can
look at the size of the table (or make it tunable) or even use a
different approach entirely. Ultimately the data store is an
implementation detail private to the audit subsystem in the kernel so
we should be able to change it as necessary without breaking anything.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH net v2] skbuff: fix compilation warnings in skb_dump()
From: Qian Cai @ 2019-07-16 15:43 UTC (permalink / raw)
To: davem; +Cc: willemb, joe, clang-built-linux, netdev, linux-kernel, Qian Cai
The commit 6413139dfc64 ("skbuff: increase verbosity when dumping skb
data") introduced a few compilation warnings.
net/core/skbuff.c:766:32: warning: format specifies type 'unsigned
short' but the argument has type 'unsigned int' [-Wformat]
level, sk->sk_family, sk->sk_type,
sk->sk_protocol);
^~~~~~~~~~~
net/core/skbuff.c:766:45: warning: format specifies type 'unsigned
short' but the argument has type 'unsigned int' [-Wformat]
level, sk->sk_family, sk->sk_type,
sk->sk_protocol);
^~~~~~~~~~~~~~~
Fix them by using the proper types.
Fixes: 6413139dfc64 ("skbuff: increase verbosity when dumping skb data")
Signed-off-by: Qian Cai <cai@lca.pw>
---
v2: Drop the checkpatch fix as it seems a bit more involved that it does not
even like passing a variable to it, i.e., printk(level "msg"). Also,
print_hex_dump() seems need a fix to be complete which can probably be done
later.
net/core/skbuff.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6f1e31f674a3..0338820ee0ec 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -762,7 +762,7 @@ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
printk("%sdev name=%s feat=0x%pNF\n",
level, dev->name, &dev->features);
if (sk)
- printk("%ssk family=%hu type=%hu proto=%hu\n",
+ printk("%ssk family=%hu type=%u proto=%u\n",
level, sk->sk_family, sk->sk_type, sk->sk_protocol);
if (full_pkt && headroom)
--
1.8.3.1
^ permalink raw reply related
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