* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 19:32 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <842190155.225.1522264944386.JavaMail.zimbra@efficios.com>
On Wed, 28 Mar 2018 15:22:24 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > > cache hot/cold argument clearly doesn't apply.
>
> In the current situation I'm fine with adding this extra field
> to struct tracepoint. However, we should keep in mind to move
> all non-required cache-cold fields to a separate section at
> some point. Clearly just this single field won't make a difference
> due to other fields and padding.
funcs is the only part of the tracepoint structure that needs hot
cache. Thus, instead of trying to keep the tracepoint structure small
for cache reasons, pull funcs out of the tracepoint structure.
What about this patch?
Of course, this just adds another 8 bytes per tracepoint :-/ But it
keeps the functions away from the tracepoint structures. We could even
add a section for them to be together, but I'm not sure that will help
much more that just letting the linker place them, as these function
pointers of the same system will probably be grouped together.
-- Steve
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index 35db8dd48c4c..8d100163e9af 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -32,7 +32,7 @@ struct tracepoint {
struct static_key key;
int (*regfunc)(void);
void (*unregfunc)(void);
- struct tracepoint_func __rcu *funcs;
+ struct tracepoint_func **funcs;
u32 num_args;
};
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index c92f4adbc0d7..b55282202f71 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -129,7 +129,7 @@ extern void syscall_unregfunc(void);
* as "(void *, void)". The DECLARE_TRACE_NOARGS() will pass in just
* "void *data", where as the DECLARE_TRACE() will pass in "void *data, proto".
*/
-#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
+#define __DO_TRACE(name, proto, args, cond, rcucheck) \
do { \
struct tracepoint_func *it_func_ptr; \
void *it_func; \
@@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
if (rcucheck) \
rcu_irq_enter_irqson(); \
rcu_read_lock_sched_notrace(); \
- it_func_ptr = rcu_dereference_sched((tp)->funcs); \
+ it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
if (it_func_ptr) { \
do { \
it_func = (it_func_ptr)->func; \
@@ -158,7 +158,7 @@ extern void syscall_unregfunc(void);
static inline void trace_##name##_rcuidle(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
- __DO_TRACE(&__tracepoint_##name, \
+ __DO_TRACE(name, \
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 1); \
@@ -181,10 +181,11 @@ extern void syscall_unregfunc(void);
*/
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
extern struct tracepoint __tracepoint_##name; \
+ extern struct tracepoint_func __rcu *__trace_##name##_funcs; \
static inline void trace_##name(proto) \
{ \
if (static_key_false(&__tracepoint_##name.key)) \
- __DO_TRACE(&__tracepoint_##name, \
+ __DO_TRACE(name, \
TP_PROTO(data_proto), \
TP_ARGS(data_args), \
TP_CONDITION(cond), 0); \
@@ -233,9 +234,11 @@ extern void syscall_unregfunc(void);
#define DEFINE_TRACE_FN(name, reg, unreg, num_args) \
static const char __tpstrtab_##name[] \
__attribute__((section("__tracepoints_strings"))) = #name; \
+ struct tracepoint_func __rcu *__trace_##name##_funcs; \
struct tracepoint __tracepoint_##name \
__attribute__((section("__tracepoints"))) = \
- { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, NULL, num_args };\
+ { __tpstrtab_##name, STATIC_KEY_INIT_FALSE, reg, unreg, \
+ &__trace_##name##_funcs, num_args }; \
static struct tracepoint * const __tracepoint_ptr_##name __used \
__attribute__((section("__tracepoints_ptrs"))) = \
&__tracepoint_##name;
@@ -244,9 +247,12 @@ extern void syscall_unregfunc(void);
DEFINE_TRACE_FN(name, NULL, NULL, num_args);
#define EXPORT_TRACEPOINT_SYMBOL_GPL(name) \
- EXPORT_SYMBOL_GPL(__tracepoint_##name)
+ EXPORT_SYMBOL_GPL(__tracepoint_##name); \
+ EXPORT_SYMBOL_GPL(__trace_##name##_funcs)
+
#define EXPORT_TRACEPOINT_SYMBOL(name) \
- EXPORT_SYMBOL(__tracepoint_##name)
+ EXPORT_SYMBOL(__tracepoint_##name); \
+ EXPORT_SYMBOL(__trace_##name##_funcs)
#else /* !TRACEPOINTS_ENABLED */
#define __DECLARE_TRACE(name, proto, args, cond, data_proto, data_args) \
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index a02bc09d765a..47b884809f22 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -809,6 +809,7 @@ static void free_synth_tracepoint(struct tracepoint *tp)
if (!tp)
return;
+ kfree(tp->funcs);
kfree(tp->name);
kfree(tp);
}
@@ -827,6 +828,13 @@ static struct tracepoint *alloc_synth_tracepoint(char *name)
return ERR_PTR(-ENOMEM);
}
+ tp->funcs = kzalloc(sizeof(*tp->funcs), GFP_KERNEL);
+ if (!tp->funcs) {
+ kfree(tp->name);
+ kfree(tp);
+ return ERR_PTR(-ENOMEM);
+ }
+
return tp;
}
@@ -846,7 +854,7 @@ static inline void trace_synth(struct synth_event *event, u64 *var_ref_vals,
if (!(cpu_online(raw_smp_processor_id())))
return;
- probe_func_ptr = rcu_dereference_sched((tp)->funcs);
+ probe_func_ptr = rcu_dereference_sched(*(tp)->funcs);
if (probe_func_ptr) {
do {
probe_func = probe_func_ptr->func;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 671b13457387..638a35f77841 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -203,7 +203,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
return ret;
}
- tp_funcs = rcu_dereference_protected(tp->funcs,
+ tp_funcs = rcu_dereference_protected(*tp->funcs,
lockdep_is_held(&tracepoints_mutex));
old = func_add(&tp_funcs, func, prio);
if (IS_ERR(old)) {
@@ -217,7 +217,7 @@ static int tracepoint_add_func(struct tracepoint *tp,
* a pointer to it. This array is referenced by __DO_TRACE from
* include/linux/tracepoint.h using rcu_dereference_sched().
*/
- rcu_assign_pointer(tp->funcs, tp_funcs);
+ rcu_assign_pointer(*tp->funcs, tp_funcs);
if (!static_key_enabled(&tp->key))
static_key_slow_inc(&tp->key);
release_probes(old);
@@ -235,7 +235,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
{
struct tracepoint_func *old, *tp_funcs;
- tp_funcs = rcu_dereference_protected(tp->funcs,
+ tp_funcs = rcu_dereference_protected(*tp->funcs,
lockdep_is_held(&tracepoints_mutex));
old = func_remove(&tp_funcs, func);
if (IS_ERR(old)) {
@@ -251,7 +251,7 @@ static int tracepoint_remove_func(struct tracepoint *tp,
if (static_key_enabled(&tp->key))
static_key_slow_dec(&tp->key);
}
- rcu_assign_pointer(tp->funcs, tp_funcs);
+ rcu_assign_pointer(*tp->funcs, tp_funcs);
release_probes(old);
return 0;
}
@@ -398,7 +398,7 @@ static void tp_module_going_check_quiescent(struct tracepoint * const *begin,
if (!begin)
return;
for (iter = begin; iter < end; iter++)
- WARN_ON_ONCE((*iter)->funcs);
+ WARN_ON_ONCE(*(*iter)->funcs);
}
static int tracepoint_module_coming(struct module *mod)
^ permalink raw reply related
* Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Keep ATU/VTU violation statistics
From: Andrew Lunn @ 2018-03-28 19:33 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev
In-Reply-To: <70d16316-df99-0664-a574-18edaa3ec313@gmail.com>
On Wed, Mar 28, 2018 at 11:17:19AM -0700, Florian Fainelli wrote:
> On 03/27/2018 02:59 PM, Andrew Lunn wrote:
> > Count the numbers of various ATU and VTU violation statistics and
> > return them as part of the ethtool -S statistics.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> > drivers/net/dsa/mv88e6xxx/chip.c | 50 ++++++++++++++++++++++++++++-----
> > drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++---
> > drivers/net/dsa/mv88e6xxx/global1_atu.c | 12 +++++---
> > drivers/net/dsa/mv88e6xxx/global1_vtu.c | 8 ++++--
> > drivers/net/dsa/mv88e6xxx/serdes.c | 15 ++++++----
> > drivers/net/dsa/mv88e6xxx/serdes.h | 8 +++---
> > 6 files changed, 78 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index 9a5d786b4885..186021f98c5d 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -723,6 +723,24 @@ static int mv88e6320_stats_get_strings(struct mv88e6xxx_chip *chip,
> > STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
> > }
> >
> > +static const uint8_t *mv88e6xxx_atu_vtu_stats_strings[] = {
>
> Why not const char *?
The ethtool call passes i uint8_t *data to receive the copy into. I'm
keeping it consistent.
> > +static void mv88e6xxx_atu_vtu_get_strings(uint8_t *data)
> > +{
> > + int i;
>
> unsigned int i?
I could do, but it seems unlikely it will overflow 31 bits.
> > +
> > + for (i = 0; i < ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings); i++)
> > + strlcpy(data + i * ETH_GSTRING_LEN,
> > + mv88e6xxx_atu_vtu_stats_strings[i],
> > + ETH_GSTRING_LEN);
> > +}
> > +
> > static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
> > uint8_t *data)
> > {
> > @@ -736,9 +754,12 @@ static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
> >
> > if (chip->info->ops->serdes_get_strings) {
> > data += count * ETH_GSTRING_LEN;
> > - chip->info->ops->serdes_get_strings(chip, port, data);
> > + count = chip->info->ops->serdes_get_strings(chip, port, data);
> > }
> >
> > + data += count * ETH_GSTRING_LEN;
> > + mv88e6xxx_atu_vtu_get_strings(data);
> > +
> > mutex_unlock(&chip->reg_lock);
> > }
> >
> > @@ -783,10 +804,13 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> > if (chip->info->ops->serdes_get_sset_count)
> > serdes_count = chip->info->ops->serdes_get_sset_count(chip,
> > port);
> > - if (serdes_count < 0)
> > + if (serdes_count < 0) {
> > count = serdes_count;
> > - else
> > - count += serdes_count;
> > + goto out;
> > + }
> > + count += serdes_count;
> > + count += ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings);
> > +
> > out:
> > mutex_unlock(&chip->reg_lock);
> >
> > @@ -841,6 +865,16 @@ static int mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
> > 0);
> > }
> >
> > +static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int port,
> > + uint64_t *data)
> > +{
> > + *data++ = chip->ports[port].atu_member_violation;
> > + *data++ = chip->ports[port].atu_miss_violation;
> > + *data++ = chip->ports[port].atu_full_violation;
> > + *data++ = chip->ports[port].vtu_member_violation;
> > + *data++ = chip->ports[port].vtu_miss_violation;
>
> This looks fine, but I suppose you could just have an u64 pointer which
> is initialized to point to atu_member_violation, and then just do
> pointer arithmetics to iterate, this would avoid possibly missing that
> function in case new ATU/VTU violations are handled in the future?
KISS. This works and is obvious.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Make VTU miss violations less spammy
From: Andrew Lunn @ 2018-03-28 19:34 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev
In-Reply-To: <f9252f0b-c78c-f377-e52f-896dca5e5a07@gmail.com>
On Wed, Mar 28, 2018 at 11:11:07AM -0700, Florian Fainelli wrote:
> On 03/27/2018 02:59 PM, Andrew Lunn wrote:
> > VTU miss violations can happen under normal conditions. Don't spam the
> > kernel log. The statistics counter will indicate it is happening, if
> > anybody is interested.
> >
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>
> Reported-by: Florian Fainelli <f.fainelli@gmail.com>
>
> > ---
> > drivers/net/dsa/mv88e6xxx/global1_vtu.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> > index 2cbaf946e7ed..e0f1b4f6e29f 100644
> > --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> > +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> > @@ -547,11 +547,9 @@ static irqreturn_t mv88e6xxx_g1_vtu_prob_irq_thread_fn(int irq, void *dev_id)
> > chip->ports[spid].vtu_member_violation++;
> > }
> >
> > - if (val & MV88E6XXX_G1_VTU_OP_MISS_VIOLATION) {
> > - dev_err_ratelimited(chip->dev, "VTU miss violation for vid %d, source port %d\n",
> > - entry.vid, spid);
>
> Why not keep it as a dev_dbg() message?
O.K, will do.
Andrew
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 19:34 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328190540.370956-7-ast@kernel.org>
On Wed, 28 Mar 2018 12:05:37 -0700
Alexei Starovoitov <ast@kernel.org> wrote:
> +++ b/include/linux/tracepoint-defs.h
> @@ -35,4 +35,10 @@ struct tracepoint {
> struct tracepoint_func __rcu *funcs;
> };
>
> +struct bpf_raw_event_map {
> + struct tracepoint *tp;
> + void *bpf_func;
> + u32 num_args;
> +} __aligned(32);
> +
If you prefer v7, I'm fine with that. For cache issues, I can pull out
the funcs from the tracepoint structure like I posted.
Mathieu, your thoughts?
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 19:38 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328153220.06a132f4@gandalf.local.home>
On Wed, 28 Mar 2018 15:32:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> -#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
> +#define __DO_TRACE(name, proto, args, cond, rcucheck) \
> do { \
> struct tracepoint_func *it_func_ptr; \
> void *it_func; \
> @@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
> if (rcucheck) \
> rcu_irq_enter_irqson(); \
> rcu_read_lock_sched_notrace(); \
> - it_func_ptr = rcu_dereference_sched((tp)->funcs); \
> + it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
What we lose in data size, we may make up for in text (which is even
more important). This will remove a dereference in the hot path.
I'll make a few builds and run size on the vmlinux images to see how
this pans out.
-- Steve
> if (it_func_ptr) { \
> do { \
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Alexei Starovoitov @ 2018-03-28 19:38 UTC (permalink / raw)
To: Steven Rostedt, Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180328153435.5e733957@gandalf.local.home>
On 3/28/18 12:34 PM, Steven Rostedt wrote:
> On Wed, 28 Mar 2018 12:05:37 -0700
> Alexei Starovoitov <ast@kernel.org> wrote:
>
>> +++ b/include/linux/tracepoint-defs.h
>> @@ -35,4 +35,10 @@ struct tracepoint {
>> struct tracepoint_func __rcu *funcs;
>> };
>>
>> +struct bpf_raw_event_map {
>> + struct tracepoint *tp;
>> + void *bpf_func;
>> + u32 num_args;
>> +} __aligned(32);
>> +
>
> If you prefer v7, I'm fine with that. For cache issues, I can pull out
> the funcs from the tracepoint structure like I posted.
I very much prefer to land this v8 as-is and optimize later.
I still have bpfilter/microkernel patches to finish which were
practically ready two weeks ago and got delayed but this set.
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Steven Rostedt @ 2018-03-28 19:40 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, davem, daniel, torvalds, peterz,
mathieu.desnoyers, netdev, kernel-team, linux-api
In-Reply-To: <2a0f6ac2-b13e-b8ad-31b4-a42909a34efa@fb.com>
On Wed, 28 Mar 2018 12:38:48 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> On 3/28/18 12:34 PM, Steven Rostedt wrote:
> > On Wed, 28 Mar 2018 12:05:37 -0700
> > Alexei Starovoitov <ast@kernel.org> wrote:
> >
> >> +++ b/include/linux/tracepoint-defs.h
> >> @@ -35,4 +35,10 @@ struct tracepoint {
> >> struct tracepoint_func __rcu *funcs;
> >> };
> >>
> >> +struct bpf_raw_event_map {
> >> + struct tracepoint *tp;
> >> + void *bpf_func;
> >> + u32 num_args;
> >> +} __aligned(32);
> >> +
> >
> > If you prefer v7, I'm fine with that. For cache issues, I can pull out
> > the funcs from the tracepoint structure like I posted.
>
> I very much prefer to land this v8 as-is and optimize later.
>
> I still have bpfilter/microkernel patches to finish which were
> practically ready two weeks ago and got delayed but this set.
Then by all means, you have my Ack. We can optimize later.
For the whole series... (v7 or v8)
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Thanks!
-- Steve
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-28 19:47 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <20180328153819.654c1600@gandalf.local.home>
----- On Mar 28, 2018, at 3:38 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 28 Mar 2018 15:32:20 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
>> -#define __DO_TRACE(tp, proto, args, cond, rcucheck) \
>> +#define __DO_TRACE(name, proto, args, cond, rcucheck) \
>> do { \
>> struct tracepoint_func *it_func_ptr; \
>> void *it_func; \
>> @@ -140,7 +140,7 @@ extern void syscall_unregfunc(void);
>> if (rcucheck) \
>> rcu_irq_enter_irqson(); \
>> rcu_read_lock_sched_notrace(); \
>> - it_func_ptr = rcu_dereference_sched((tp)->funcs); \
>> + it_func_ptr = rcu_dereference_sched(__trace_##name##_funcs); \
>
> What we lose in data size, we may make up for in text (which is even
> more important). This will remove a dereference in the hot path.
>
> I'll make a few builds and run size on the vmlinux images to see how
> this pans out.
I like the approach. If it passes testing, I think it's a valuable improvement
to lessen cache footprint of tracepoint when tracing is active.
Thanks!
Mathieu
>
> -- Steve
>
>
>> if (it_func_ptr) { \
> > do { \
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v8 bpf-next 6/9] bpf: introduce BPF_RAW_TRACEPOINT
From: Mathieu Desnoyers @ 2018-03-28 19:48 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, Alexei Starovoitov, David S. Miller,
Daniel Borkmann, Linus Torvalds, Peter Zijlstra, netdev,
kernel-team, linux-api
In-Reply-To: <20180328154027.569206ba@gandalf.local.home>
----- On Mar 28, 2018, at 3:40 PM, rostedt rostedt@goodmis.org wrote:
> On Wed, 28 Mar 2018 12:38:48 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> On 3/28/18 12:34 PM, Steven Rostedt wrote:
>> > On Wed, 28 Mar 2018 12:05:37 -0700
>> > Alexei Starovoitov <ast@kernel.org> wrote:
>> >
>> >> +++ b/include/linux/tracepoint-defs.h
>> >> @@ -35,4 +35,10 @@ struct tracepoint {
>> >> struct tracepoint_func __rcu *funcs;
>> >> };
>> >>
>> >> +struct bpf_raw_event_map {
>> >> + struct tracepoint *tp;
>> >> + void *bpf_func;
>> >> + u32 num_args;
>> >> +} __aligned(32);
>> >> +
>> >
>> > If you prefer v7, I'm fine with that. For cache issues, I can pull out
>> > the funcs from the tracepoint structure like I posted.
>>
>> I very much prefer to land this v8 as-is and optimize later.
>>
>> I still have bpfilter/microkernel patches to finish which were
>> practically ready two weeks ago and got delayed but this set.
>
> Then by all means, you have my Ack. We can optimize later.
>
> For the whole series... (v7 or v8)
>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
I'm fine with either v7 or v8 as well.
Thanks,
Mathieu
>
> Thanks!
>
> -- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* [bpf-next PATCH v3 0/4] bpf, sockmap BPF_F_INGRESS support
From: John Fastabend @ 2018-03-28 19:49 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
This series adds the BPF_F_INGRESS flag support to the redirect APIs.
Bringing the sockmap API in-line with the cls_bpf redirect APIs.
We add it to both variants of sockmap programs, the first patch adds
support for tx ulp hooks and the third patch adds support for the recv
skb hooks. Patches two and four add tests for the corresponding
ingress redirect hooks.
Follow on patches can address busy polling support, but next series
from me will move the sockmap sample program into selftests.
v2: added static to function definition caught by kbuild bot
v3: fixed an error branch with missing mem_uncharge
in recvmsg op moved receive_queue check outside of RCU region
---
John Fastabend (4):
bpf: sockmap redirect ingress support
bpf: sockmap, add BPF_F_INGRESS tests
bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
bpf: sockmap, more BPF_SK_SKB_STREAM_VERDICT tests
include/linux/filter.h | 2
include/net/sock.h | 1
kernel/bpf/sockmap.c | 290 ++++++++++++++++++++++++++++++++++++---
net/core/filter.c | 4 -
net/ipv4/tcp.c | 10 +
samples/sockmap/sockmap_kern.c | 62 +++++++-
samples/sockmap/sockmap_test.sh | 40 +++++
samples/sockmap/sockmap_user.c | 58 ++++++++
8 files changed, 430 insertions(+), 37 deletions(-)
^ permalink raw reply
* [bpf-next PATCH v3 1/4] bpf: sockmap redirect ingress support
From: John Fastabend @ 2018-03-28 19:49 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180328194710.16072.99278.stgit@john-Precision-Tower-5810>
Add support for the BPF_F_INGRESS flag in sk_msg redirect helper.
To do this add a scatterlist ring for receiving socks to check
before calling into regular recvmsg call path. Additionally, because
the poll wakeup logic only checked the skb recv queue we need to
add a hook in TCP stack (similar to write side) so that we have
a way to wake up polling socks when a scatterlist is redirected
to that sock.
After this all that is needed is for the redirect helper to
push the scatterlist into the psock receive queue.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/linux/filter.h | 1
include/net/sock.h | 1
kernel/bpf/sockmap.c | 198 +++++++++++++++++++++++++++++++++++++++++++++++-
net/core/filter.c | 2
net/ipv4/tcp.c | 10 ++
5 files changed, 207 insertions(+), 5 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 109d05c..d0e207f 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -521,6 +521,7 @@ struct sk_msg_buff {
__u32 key;
__u32 flags;
struct bpf_map *map;
+ struct list_head list;
};
/* Compute the linear packet data range [data, data_end) which
diff --git a/include/net/sock.h b/include/net/sock.h
index 7093111..b8ff435 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1085,6 +1085,7 @@ struct proto {
#endif
bool (*stream_memory_free)(const struct sock *sk);
+ bool (*stream_memory_read)(const struct sock *sk);
/* Memory pressure */
void (*enter_memory_pressure)(struct sock *sk);
void (*leave_memory_pressure)(struct sock *sk);
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 69c5bcc..402e154 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -41,6 +41,8 @@
#include <linux/mm.h>
#include <net/strparser.h>
#include <net/tcp.h>
+#include <linux/ptr_ring.h>
+#include <net/inet_common.h>
#define SOCK_CREATE_FLAG_MASK \
(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
@@ -82,6 +84,7 @@ struct smap_psock {
int sg_size;
int eval;
struct sk_msg_buff *cork;
+ struct list_head ingress;
struct strparser strp;
struct bpf_prog *bpf_tx_msg;
@@ -103,6 +106,8 @@ struct smap_psock {
};
static void smap_release_sock(struct smap_psock *psock, struct sock *sock);
+static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
+ int nonblock, int flags, int *addr_len);
static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size);
static int bpf_tcp_sendpage(struct sock *sk, struct page *page,
int offset, size_t size, int flags);
@@ -112,6 +117,21 @@ static inline struct smap_psock *smap_psock_sk(const struct sock *sk)
return rcu_dereference_sk_user_data(sk);
}
+static bool bpf_tcp_stream_read(const struct sock *sk)
+{
+ struct smap_psock *psock;
+ bool empty = true;
+
+ rcu_read_lock();
+ psock = smap_psock_sk(sk);
+ if (unlikely(!psock))
+ goto out;
+ empty = list_empty(&psock->ingress);
+out:
+ rcu_read_unlock();
+ return !empty;
+}
+
static struct proto tcp_bpf_proto;
static int bpf_tcp_init(struct sock *sk)
{
@@ -135,6 +155,8 @@ static int bpf_tcp_init(struct sock *sk)
if (psock->bpf_tx_msg) {
tcp_bpf_proto.sendmsg = bpf_tcp_sendmsg;
tcp_bpf_proto.sendpage = bpf_tcp_sendpage;
+ tcp_bpf_proto.recvmsg = bpf_tcp_recvmsg;
+ tcp_bpf_proto.stream_memory_read = bpf_tcp_stream_read;
}
sk->sk_prot = &tcp_bpf_proto;
@@ -170,6 +192,7 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
{
void (*close_fun)(struct sock *sk, long timeout);
struct smap_psock_map_entry *e, *tmp;
+ struct sk_msg_buff *md, *mtmp;
struct smap_psock *psock;
struct sock *osk;
@@ -188,6 +211,12 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
close_fun = psock->save_close;
write_lock_bh(&sk->sk_callback_lock);
+ list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
+ list_del(&md->list);
+ free_start_sg(psock->sock, md);
+ kfree(md);
+ }
+
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
osk = cmpxchg(e->entry, sk, NULL);
if (osk == sk) {
@@ -468,6 +497,72 @@ static unsigned int smap_do_tx_msg(struct sock *sk,
return _rc;
}
+static int bpf_tcp_ingress(struct sock *sk, int apply_bytes,
+ struct smap_psock *psock,
+ struct sk_msg_buff *md, int flags)
+{
+ bool apply = apply_bytes;
+ size_t size, copied = 0;
+ struct sk_msg_buff *r;
+ int err = 0, i;
+
+ r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_KERNEL);
+ if (unlikely(!r))
+ return -ENOMEM;
+
+ lock_sock(sk);
+ r->sg_start = md->sg_start;
+ i = md->sg_start;
+
+ do {
+ r->sg_data[i] = md->sg_data[i];
+
+ size = (apply && apply_bytes < md->sg_data[i].length) ?
+ apply_bytes : md->sg_data[i].length;
+
+ if (!sk_wmem_schedule(sk, size)) {
+ if (!copied)
+ err = -ENOMEM;
+ break;
+ }
+
+ sk_mem_charge(sk, size);
+ r->sg_data[i].length = size;
+ md->sg_data[i].length -= size;
+ md->sg_data[i].offset += size;
+ copied += size;
+
+ if (md->sg_data[i].length) {
+ get_page(sg_page(&r->sg_data[i]));
+ r->sg_end = (i + 1) == MAX_SKB_FRAGS ? 0 : i + 1;
+ } else {
+ i++;
+ if (i == MAX_SKB_FRAGS)
+ i = 0;
+ r->sg_end = i;
+ }
+
+ if (apply) {
+ apply_bytes -= size;
+ if (!apply_bytes)
+ break;
+ }
+ } while (i != md->sg_end);
+
+ md->sg_start = i;
+
+ if (!err) {
+ list_add_tail(&r->list, &psock->ingress);
+ sk->sk_data_ready(sk);
+ } else {
+ free_start_sg(sk, r);
+ kfree(r);
+ }
+
+ release_sock(sk);
+ return err;
+}
+
static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
struct sk_msg_buff *md,
int flags)
@@ -475,6 +570,7 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
struct smap_psock *psock;
struct scatterlist *sg;
int i, err, free = 0;
+ bool ingress = !!(md->flags & BPF_F_INGRESS);
sg = md->sg_data;
@@ -487,9 +583,14 @@ static int bpf_tcp_sendmsg_do_redirect(struct sock *sk, int send,
goto out_rcu;
rcu_read_unlock();
- lock_sock(sk);
- err = bpf_tcp_push(sk, send, md, flags, false);
- release_sock(sk);
+
+ if (ingress) {
+ err = bpf_tcp_ingress(sk, send, psock, md, flags);
+ } else {
+ lock_sock(sk);
+ err = bpf_tcp_push(sk, send, md, flags, false);
+ release_sock(sk);
+ }
smap_release_sock(psock, sk);
if (unlikely(err))
goto out;
@@ -623,6 +724,89 @@ static int bpf_exec_tx_verdict(struct smap_psock *psock,
return err;
}
+static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
+ int nonblock, int flags, int *addr_len)
+{
+ struct iov_iter *iter = &msg->msg_iter;
+ struct smap_psock *psock;
+ int copied = 0;
+
+ if (unlikely(flags & MSG_ERRQUEUE))
+ return inet_recv_error(sk, msg, len, addr_len);
+
+ rcu_read_lock();
+ psock = smap_psock_sk(sk);
+ if (unlikely(!psock))
+ goto out;
+
+ if (unlikely(!refcount_inc_not_zero(&psock->refcnt)))
+ goto out;
+ rcu_read_unlock();
+
+ if (!skb_queue_empty(&sk->sk_receive_queue))
+ return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
+
+ lock_sock(sk);
+ while (copied != len) {
+ struct scatterlist *sg;
+ struct sk_msg_buff *md;
+ int i;
+
+ md = list_first_entry_or_null(&psock->ingress,
+ struct sk_msg_buff, list);
+ if (unlikely(!md))
+ break;
+ i = md->sg_start;
+ do {
+ struct page *page;
+ int n, copy;
+
+ sg = &md->sg_data[i];
+ copy = sg->length;
+ page = sg_page(sg);
+
+ if (copied + copy > len)
+ copy = len - copied;
+
+ n = copy_page_to_iter(page, sg->offset, copy, iter);
+ if (n != copy) {
+ md->sg_start = i;
+ release_sock(sk);
+ smap_release_sock(psock, sk);
+ return -EFAULT;
+ }
+
+ copied += copy;
+ sg->offset += copy;
+ sg->length -= copy;
+ sk_mem_uncharge(sk, copy);
+
+ if (!sg->length) {
+ i++;
+ if (i == MAX_SKB_FRAGS)
+ i = 0;
+ put_page(page);
+ }
+ if (copied == len)
+ break;
+ } while (i != md->sg_end);
+ md->sg_start = i;
+
+ if (!sg->length && md->sg_start == md->sg_end) {
+ list_del(&md->list);
+ kfree(md);
+ }
+ }
+
+ release_sock(sk);
+ smap_release_sock(psock, sk);
+ return copied;
+out:
+ rcu_read_unlock();
+ return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len);
+}
+
+
static int bpf_tcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
{
int flags = msg->msg_flags | MSG_NO_SHARED_FRAGS;
@@ -1107,6 +1291,7 @@ static void sock_map_remove_complete(struct bpf_stab *stab)
static void smap_gc_work(struct work_struct *w)
{
struct smap_psock_map_entry *e, *tmp;
+ struct sk_msg_buff *md, *mtmp;
struct smap_psock *psock;
psock = container_of(w, struct smap_psock, gc_work);
@@ -1131,6 +1316,12 @@ static void smap_gc_work(struct work_struct *w)
kfree(psock->cork);
}
+ list_for_each_entry_safe(md, mtmp, &psock->ingress, list) {
+ list_del(&md->list);
+ free_start_sg(psock->sock, md);
+ kfree(md);
+ }
+
list_for_each_entry_safe(e, tmp, &psock->maps, list) {
list_del(&e->list);
kfree(e);
@@ -1160,6 +1351,7 @@ static struct smap_psock *smap_init_psock(struct sock *sock,
INIT_WORK(&psock->tx_work, smap_tx_work);
INIT_WORK(&psock->gc_work, smap_gc_work);
INIT_LIST_HEAD(&psock->maps);
+ INIT_LIST_HEAD(&psock->ingress);
refcount_set(&psock->refcnt, 1);
rcu_assign_sk_user_data(sock, psock);
diff --git a/net/core/filter.c b/net/core/filter.c
index 00c711c..11b1f16 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1894,7 +1894,7 @@ struct sock *do_sk_redirect_map(struct sk_buff *skb)
struct bpf_map *, map, u32, key, u64, flags)
{
/* If user passes invalid input drop the packet. */
- if (unlikely(flags))
+ if (unlikely(flags & ~(BPF_F_INGRESS)))
return SK_DROP;
msg->key = key;
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0c31be3..bccc4c2 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -485,6 +485,14 @@ static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
}
}
+static inline bool tcp_stream_is_readable(const struct tcp_sock *tp,
+ int target, struct sock *sk)
+{
+ return (tp->rcv_nxt - tp->copied_seq >= target) ||
+ (sk->sk_prot->stream_memory_read ?
+ sk->sk_prot->stream_memory_read(sk) : false);
+}
+
/*
* Wait for a TCP event.
*
@@ -554,7 +562,7 @@ __poll_t tcp_poll(struct file *file, struct socket *sock, poll_table *wait)
tp->urg_data)
target++;
- if (tp->rcv_nxt - tp->copied_seq >= target)
+ if (tcp_stream_is_readable(tp, target, sk))
mask |= EPOLLIN | EPOLLRDNORM;
if (!(sk->sk_shutdown & SEND_SHUTDOWN)) {
^ permalink raw reply related
* [bpf-next PATCH v3 2/4] bpf: sockmap, add BPF_F_INGRESS tests
From: John Fastabend @ 2018-03-28 19:49 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180328194710.16072.99278.stgit@john-Precision-Tower-5810>
Add a set of tests to verify ingress flag in redirect helpers
works correctly with various msg sizes.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
samples/sockmap/sockmap_kern.c | 41 +++++++++++++++++++++++++++++----------
samples/sockmap/sockmap_test.sh | 22 ++++++++++++++++++++-
samples/sockmap/sockmap_user.c | 35 +++++++++++++++++++++++++++++++++
3 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/samples/sockmap/sockmap_kern.c b/samples/sockmap/sockmap_kern.c
index 9ad5ba7..ca28722 100644
--- a/samples/sockmap/sockmap_kern.c
+++ b/samples/sockmap/sockmap_kern.c
@@ -54,7 +54,7 @@ struct bpf_map_def SEC("maps") sock_map_redir = {
.type = BPF_MAP_TYPE_SOCKMAP,
.key_size = sizeof(int),
.value_size = sizeof(int),
- .max_entries = 1,
+ .max_entries = 20,
};
struct bpf_map_def SEC("maps") sock_apply_bytes = {
@@ -78,6 +78,13 @@ struct bpf_map_def SEC("maps") sock_pull_bytes = {
.max_entries = 2
};
+struct bpf_map_def SEC("maps") sock_redir_flags = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 1
+};
+
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
@@ -197,8 +204,9 @@ int bpf_prog5(struct sk_msg_md *msg)
SEC("sk_msg3")
int bpf_prog6(struct sk_msg_md *msg)
{
- int *bytes, zero = 0, one = 1;
- int *start, *end;
+ int *bytes, zero = 0, one = 1, key = 0;
+ int *start, *end, *f;
+ __u64 flags = 0;
bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
if (bytes)
@@ -210,15 +218,22 @@ int bpf_prog6(struct sk_msg_md *msg)
end = bpf_map_lookup_elem(&sock_pull_bytes, &one);
if (start && end)
bpf_msg_pull_data(msg, *start, *end, 0);
- return bpf_msg_redirect_map(msg, &sock_map_redir, zero, 0);
+ f = bpf_map_lookup_elem(&sock_redir_flags, &zero);
+ if (f && *f) {
+ key = 2;
+ flags = *f;
+ }
+ return bpf_msg_redirect_map(msg, &sock_map_redir, key, flags);
}
SEC("sk_msg4")
int bpf_prog7(struct sk_msg_md *msg)
{
- int err1 = 0, err2 = 0, zero = 0, one = 1;
- int *bytes, *start, *end, len1, len2;
+ int err1 = 0, err2 = 0, zero = 0, one = 1, key = 0;
+ int *f, *bytes, *start, *end, len1, len2;
+ __u64 flags = 0;
+ int err;
bytes = bpf_map_lookup_elem(&sock_apply_bytes, &zero);
if (bytes)
err1 = bpf_msg_apply_bytes(msg, *bytes);
@@ -229,7 +244,6 @@ int bpf_prog7(struct sk_msg_md *msg)
start = bpf_map_lookup_elem(&sock_pull_bytes, &zero);
end = bpf_map_lookup_elem(&sock_pull_bytes, &one);
if (start && end) {
- int err;
bpf_printk("sk_msg2: pull(%i:%i)\n",
start ? *start : 0, end ? *end : 0);
@@ -241,9 +255,16 @@ int bpf_prog7(struct sk_msg_md *msg)
bpf_printk("sk_msg2: length update %i->%i\n",
len1, len2);
}
- bpf_printk("sk_msg3: redirect(%iB) err1=%i err2=%i\n",
- len1, err1, err2);
- return bpf_msg_redirect_map(msg, &sock_map_redir, zero, 0);
+ f = bpf_map_lookup_elem(&sock_redir_flags, &zero);
+ if (f && *f) {
+ key = 2;
+ flags = *f;
+ }
+ bpf_printk("sk_msg3: redirect(%iB) flags=%i err=%i\n",
+ len1, flags, err1 ? err1 : err2);
+ err = bpf_msg_redirect_map(msg, &sock_map_redir, key, flags);
+ bpf_printk("sk_msg3: err %i\n", err);
+ return err;
}
SEC("sk_msg5")
diff --git a/samples/sockmap/sockmap_test.sh b/samples/sockmap/sockmap_test.sh
index 6d8cc40..13b205f 100755
--- a/samples/sockmap/sockmap_test.sh
+++ b/samples/sockmap/sockmap_test.sh
@@ -1,5 +1,5 @@
#Test a bunch of positive cases to verify basic functionality
-for prog in "--txmsg" "--txmsg_redir" "--txmsg_drop"; do
+for prog in "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
for t in "sendmsg" "sendpage"; do
for r in 1 10 100; do
for i in 1 10 100; do
@@ -100,6 +100,16 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 1 --txmsg_ingress"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
+
# Test apply and redirect with larger value than send
r=1
i=8
@@ -113,6 +123,16 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 2048 --txmsg_ingress"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
+
# Test apply and redirect with apply that never reaches limit
r=1024
i=1
diff --git a/samples/sockmap/sockmap_user.c b/samples/sockmap/sockmap_user.c
index 07aa237..f7503f4 100644
--- a/samples/sockmap/sockmap_user.c
+++ b/samples/sockmap/sockmap_user.c
@@ -64,6 +64,7 @@
int txmsg_cork;
int txmsg_start;
int txmsg_end;
+int txmsg_ingress;
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
@@ -83,6 +84,7 @@
{"txmsg_cork", required_argument, NULL, 'k'},
{"txmsg_start", required_argument, NULL, 's'},
{"txmsg_end", required_argument, NULL, 'e'},
+ {"txmsg_ingress", no_argument, &txmsg_ingress, 1 },
{0, 0, NULL, 0 }
};
@@ -793,6 +795,39 @@ int main(int argc, char **argv)
return err;
}
}
+
+ if (txmsg_ingress) {
+ int in = BPF_F_INGRESS;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd[6], &i, &in, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (txmsg_ingress): %d (%s)\n",
+ err, strerror(errno));
+ }
+ i = 1;
+ err = bpf_map_update_elem(map_fd[1], &i, &p1, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p1 txmsg): %d (%s)\n",
+ err, strerror(errno));
+ }
+ err = bpf_map_update_elem(map_fd[2], &i, &p1, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p1 redir): %d (%s)\n",
+ err, strerror(errno));
+ }
+
+ i = 2;
+ err = bpf_map_update_elem(map_fd[2], &i, &p2, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (p2 txmsg): %d (%s)\n",
+ err, strerror(errno));
+ }
+ }
}
if (txmsg_drop)
^ permalink raw reply related
* [bpf-next PATCH v3 3/4] bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
From: John Fastabend @ 2018-03-28 19:49 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180328194710.16072.99278.stgit@john-Precision-Tower-5810>
Add support for the BPF_F_INGRESS flag in skb redirect helper. To
do this convert skb into a scatterlist and push into ingress queue.
This is the same logic that is used in the sk_msg redirect helper
so it should feel familiar.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/linux/filter.h | 1 +
kernel/bpf/sockmap.c | 94 +++++++++++++++++++++++++++++++++++++++---------
net/core/filter.c | 2 +
3 files changed, 78 insertions(+), 19 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index d0e207f..7de778a 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -521,6 +521,7 @@ struct sk_msg_buff {
__u32 key;
__u32 flags;
struct bpf_map *map;
+ struct sk_buff *skb;
struct list_head list;
};
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 402e154..9192fdb 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -785,7 +785,8 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
i++;
if (i == MAX_SKB_FRAGS)
i = 0;
- put_page(page);
+ if (!md->skb)
+ put_page(page);
}
if (copied == len)
break;
@@ -794,6 +795,8 @@ static int bpf_tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
if (!sg->length && md->sg_start == md->sg_end) {
list_del(&md->list);
+ if (md->skb)
+ consume_skb(md->skb);
kfree(md);
}
}
@@ -1045,27 +1048,72 @@ static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
__SK_DROP;
}
+static int smap_do_ingress(struct smap_psock *psock, struct sk_buff *skb)
+{
+ struct sock *sk = psock->sock;
+ int copied = 0, num_sg;
+ struct sk_msg_buff *r;
+
+ r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_ATOMIC);
+ if (unlikely(!r))
+ return -EAGAIN;
+
+ if (!sk_rmem_schedule(sk, skb, skb->len)) {
+ kfree(r);
+ return -EAGAIN;
+ }
+
+ sg_init_table(r->sg_data, MAX_SKB_FRAGS);
+ num_sg = skb_to_sgvec(skb, r->sg_data, 0, skb->len);
+ if (unlikely(num_sg < 0)) {
+ kfree(r);
+ return num_sg;
+ }
+ sk_mem_charge(sk, skb->len);
+ copied = skb->len;
+ r->sg_start = 0;
+ r->sg_end = num_sg == MAX_SKB_FRAGS ? 0 : num_sg;
+ r->skb = skb;
+ list_add_tail(&r->list, &psock->ingress);
+ sk->sk_data_ready(sk);
+ return copied;
+}
+
static void smap_do_verdict(struct smap_psock *psock, struct sk_buff *skb)
{
+ struct smap_psock *peer;
struct sock *sk;
+ __u32 in;
int rc;
rc = smap_verdict_func(psock, skb);
switch (rc) {
case __SK_REDIRECT:
sk = do_sk_redirect_map(skb);
- if (likely(sk)) {
- struct smap_psock *peer = smap_psock_sk(sk);
-
- if (likely(peer &&
- test_bit(SMAP_TX_RUNNING, &peer->state) &&
- !sock_flag(sk, SOCK_DEAD) &&
- sock_writeable(sk))) {
- skb_set_owner_w(skb, sk);
- skb_queue_tail(&peer->rxqueue, skb);
- schedule_work(&peer->tx_work);
- break;
- }
+ if (!sk) {
+ kfree_skb(skb);
+ break;
+ }
+
+ peer = smap_psock_sk(sk);
+ in = (TCP_SKB_CB(skb)->bpf.flags) & BPF_F_INGRESS;
+
+ if (unlikely(!peer || sock_flag(sk, SOCK_DEAD) ||
+ !test_bit(SMAP_TX_RUNNING, &peer->state))) {
+ kfree_skb(skb);
+ break;
+ }
+
+ if (!in && sock_writeable(sk)) {
+ skb_set_owner_w(skb, sk);
+ skb_queue_tail(&peer->rxqueue, skb);
+ schedule_work(&peer->tx_work);
+ break;
+ } else if (in &&
+ atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf) {
+ skb_queue_tail(&peer->rxqueue, skb);
+ schedule_work(&peer->tx_work);
+ break;
}
/* Fall through and free skb otherwise */
case __SK_DROP:
@@ -1127,15 +1175,23 @@ static void smap_tx_work(struct work_struct *w)
}
while ((skb = skb_dequeue(&psock->rxqueue))) {
+ __u32 flags;
+
rem = skb->len;
off = 0;
start:
+ flags = (TCP_SKB_CB(skb)->bpf.flags) & BPF_F_INGRESS;
do {
- if (likely(psock->sock->sk_socket))
- n = skb_send_sock_locked(psock->sock,
- skb, off, rem);
- else
+ if (likely(psock->sock->sk_socket)) {
+ if (flags)
+ n = smap_do_ingress(psock, skb);
+ else
+ n = skb_send_sock_locked(psock->sock,
+ skb, off, rem);
+ } else {
n = -EINVAL;
+ }
+
if (n <= 0) {
if (n == -EAGAIN) {
/* Retry when space is available */
@@ -1153,7 +1209,9 @@ static void smap_tx_work(struct work_struct *w)
rem -= n;
off += n;
} while (rem);
- kfree_skb(skb);
+
+ if (!flags)
+ kfree_skb(skb);
}
out:
release_sock(psock->sock);
diff --git a/net/core/filter.c b/net/core/filter.c
index 11b1f16..b46916d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1855,7 +1855,7 @@ int skb_do_redirect(struct sk_buff *skb)
struct tcp_skb_cb *tcb = TCP_SKB_CB(skb);
/* If user passes invalid input drop the packet. */
- if (unlikely(flags))
+ if (unlikely(flags & ~(BPF_F_INGRESS)))
return SK_DROP;
tcb->bpf.key = key;
^ permalink raw reply related
* [bpf-next PATCH v3 4/4] bpf: sockmap, more BPF_SK_SKB_STREAM_VERDICT tests
From: John Fastabend @ 2018-03-28 19:49 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, davem
In-Reply-To: <20180328194710.16072.99278.stgit@john-Precision-Tower-5810>
Add BPF_SK_SKB_STREAM_VERDICT tests for ingress hook. While
we do this also bring stream tests in-line with MSG based
testing.
A map for skb options is added for userland to push options
at BPF programs.
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
samples/sockmap/sockmap_kern.c | 21 ++++++++++++++++++---
samples/sockmap/sockmap_test.sh | 20 +++++++++++++++++++-
samples/sockmap/sockmap_user.c | 23 +++++++++++++++++++++++
3 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/samples/sockmap/sockmap_kern.c b/samples/sockmap/sockmap_kern.c
index ca28722..9ff8bc5 100644
--- a/samples/sockmap/sockmap_kern.c
+++ b/samples/sockmap/sockmap_kern.c
@@ -85,6 +85,12 @@ struct bpf_map_def SEC("maps") sock_redir_flags = {
.max_entries = 1
};
+struct bpf_map_def SEC("maps") sock_skb_opts = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(int),
+ .max_entries = 1
+};
SEC("sk_skb1")
int bpf_prog1(struct __sk_buff *skb)
@@ -97,15 +103,24 @@ int bpf_prog2(struct __sk_buff *skb)
{
__u32 lport = skb->local_port;
__u32 rport = skb->remote_port;
- int ret = 0;
+ int len, *f, ret, zero = 0;
+ __u64 flags = 0;
if (lport == 10000)
ret = 10;
else
ret = 1;
- bpf_printk("sockmap: %d -> %d @ %d\n", lport, bpf_ntohl(rport), ret);
- return bpf_sk_redirect_map(skb, &sock_map, ret, 0);
+ len = (__u32)skb->data_end - (__u32)skb->data;
+ f = bpf_map_lookup_elem(&sock_skb_opts, &zero);
+ if (f && *f) {
+ ret = 3;
+ flags = *f;
+ }
+
+ bpf_printk("sk_skb2: redirect(%iB) flags=%i\n",
+ len, flags);
+ return bpf_sk_redirect_map(skb, &sock_map, ret, flags);
}
SEC("sockops")
diff --git a/samples/sockmap/sockmap_test.sh b/samples/sockmap/sockmap_test.sh
index 13b205f..ace75f0 100755
--- a/samples/sockmap/sockmap_test.sh
+++ b/samples/sockmap/sockmap_test.sh
@@ -1,5 +1,5 @@
#Test a bunch of positive cases to verify basic functionality
-for prog in "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
+for prog in "--txmsg_redir --txmsg_skb" "--txmsg_redir --txmsg_ingress" "--txmsg" "--txmsg_redir" "--txmsg_redir --txmsg_ingress" "--txmsg_drop"; do
for t in "sendmsg" "sendpage"; do
for r in 1 10 100; do
for i in 1 10 100; do
@@ -109,6 +109,15 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 1 --txmsg_skb"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
# Test apply and redirect with larger value than send
r=1
@@ -132,6 +141,15 @@ for t in "sendmsg" "sendpage"; do
sleep 2
done
+prog="--txmsg_redir --txmsg_apply 2048 --txmsg_skb"
+
+for t in "sendmsg" "sendpage"; do
+ TEST="./sockmap --cgroup /mnt/cgroup2/ -t $t -r $r -i $i -l $l $prog"
+ echo $TEST
+ $TEST
+ sleep 2
+done
+
# Test apply and redirect with apply that never reaches limit
r=1024
diff --git a/samples/sockmap/sockmap_user.c b/samples/sockmap/sockmap_user.c
index f7503f4..6f23349 100644
--- a/samples/sockmap/sockmap_user.c
+++ b/samples/sockmap/sockmap_user.c
@@ -65,6 +65,7 @@
int txmsg_start;
int txmsg_end;
int txmsg_ingress;
+int txmsg_skb;
static const struct option long_options[] = {
{"help", no_argument, NULL, 'h' },
@@ -85,6 +86,7 @@
{"txmsg_start", required_argument, NULL, 's'},
{"txmsg_end", required_argument, NULL, 'e'},
{"txmsg_ingress", no_argument, &txmsg_ingress, 1 },
+ {"txmsg_skb", no_argument, &txmsg_skb, 1 },
{0, 0, NULL, 0 }
};
@@ -828,6 +830,27 @@ int main(int argc, char **argv)
err, strerror(errno));
}
}
+
+ if (txmsg_skb) {
+ int skb_fd = (test == SENDMSG || test == SENDPAGE) ? p2 : p1;
+ int ingress = BPF_F_INGRESS;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd[7], &i, &ingress, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (txmsg_ingress): %d (%s)\n",
+ err, strerror(errno));
+ }
+
+ i = 3;
+ err = bpf_map_update_elem(map_fd[0], &i, &skb_fd, BPF_ANY);
+ if (err) {
+ fprintf(stderr,
+ "ERROR: bpf_map_update_elem (c1 sockmap): %d (%s)\n",
+ err, strerror(errno));
+ }
+ }
}
if (txmsg_drop)
^ permalink raw reply related
* Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Keep ATU/VTU violation statistics
From: Florian Fainelli @ 2018-03-28 19:57 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev
In-Reply-To: <20180328193304.GB20749@lunn.ch>
On 03/28/2018 12:33 PM, Andrew Lunn wrote:
> On Wed, Mar 28, 2018 at 11:17:19AM -0700, Florian Fainelli wrote:
>> On 03/27/2018 02:59 PM, Andrew Lunn wrote:
>>> Count the numbers of various ATU and VTU violation statistics and
>>> return them as part of the ethtool -S statistics.
>>>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>> ---
>>> drivers/net/dsa/mv88e6xxx/chip.c | 50 ++++++++++++++++++++++++++++-----
>>> drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++---
>>> drivers/net/dsa/mv88e6xxx/global1_atu.c | 12 +++++---
>>> drivers/net/dsa/mv88e6xxx/global1_vtu.c | 8 ++++--
>>> drivers/net/dsa/mv88e6xxx/serdes.c | 15 ++++++----
>>> drivers/net/dsa/mv88e6xxx/serdes.h | 8 +++---
>>> 6 files changed, 78 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>>> index 9a5d786b4885..186021f98c5d 100644
>>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>>> @@ -723,6 +723,24 @@ static int mv88e6320_stats_get_strings(struct mv88e6xxx_chip *chip,
>>> STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
>>> }
>>>
>>> +static const uint8_t *mv88e6xxx_atu_vtu_stats_strings[] = {
>>
>> Why not const char *?
>
> The ethtool call passes i uint8_t *data to receive the copy into. I'm
> keeping it consistent.
Fair enough.
>
>>> +static void mv88e6xxx_atu_vtu_get_strings(uint8_t *data)
>>> +{
>>> + int i;
>>
>> unsigned int i?
>
> I could do, but it seems unlikely it will overflow 31 bits.
The size cannot be negative, so unsigned int would seem like a natural
choice.
>
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings); i++)
>>> + strlcpy(data + i * ETH_GSTRING_LEN,
>>> + mv88e6xxx_atu_vtu_stats_strings[i],
>>> + ETH_GSTRING_LEN);
>>> +}
>>> +
>>> static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
>>> uint8_t *data)
>>> {
>>> @@ -736,9 +754,12 @@ static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
>>>
>>> if (chip->info->ops->serdes_get_strings) {
>>> data += count * ETH_GSTRING_LEN;
>>> - chip->info->ops->serdes_get_strings(chip, port, data);
>>> + count = chip->info->ops->serdes_get_strings(chip, port, data);
>>> }
>>>
>>> + data += count * ETH_GSTRING_LEN;
>>> + mv88e6xxx_atu_vtu_get_strings(data);
>>> +
>>> mutex_unlock(&chip->reg_lock);
>>> }
>>>
>>> @@ -783,10 +804,13 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>>> if (chip->info->ops->serdes_get_sset_count)
>>> serdes_count = chip->info->ops->serdes_get_sset_count(chip,
>>> port);
>>> - if (serdes_count < 0)
>>> + if (serdes_count < 0) {
>>> count = serdes_count;
>>> - else
>>> - count += serdes_count;
>>> + goto out;
>>> + }
>>> + count += serdes_count;
>>> + count += ARRAY_SIZE(mv88e6xxx_atu_vtu_stats_strings);
>>> +
>>> out:
>>> mutex_unlock(&chip->reg_lock);
>>>
>>> @@ -841,6 +865,16 @@ static int mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>>> 0);
>>> }
>>>
>>> +static void mv88e6xxx_atu_vtu_get_stats(struct mv88e6xxx_chip *chip, int port,
>>> + uint64_t *data)
>>> +{
>>> + *data++ = chip->ports[port].atu_member_violation;
>>> + *data++ = chip->ports[port].atu_miss_violation;
>>> + *data++ = chip->ports[port].atu_full_violation;
>>> + *data++ = chip->ports[port].vtu_member_violation;
>>> + *data++ = chip->ports[port].vtu_miss_violation;
>>
>> This looks fine, but I suppose you could just have an u64 pointer which
>> is initialized to point to atu_member_violation, and then just do
>> pointer arithmetics to iterate, this would avoid possibly missing that
>> function in case new ATU/VTU violations are handled in the future?
>
> KISS. This works and is obvious.
Fair enough.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: systemport: Remove adaptive TX coalescing
From: Tal Gilboa @ 2018-03-28 20:51 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, Michael Chan, gospo,
saeedm
In-Reply-To: <20180327194707.31857-2-f.fainelli@gmail.com>
On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> Adaptive TX coalescing is not currently giving us any advantages and
> ends up making the CPU spin more frequently until TX completion. Deny
> and disable adaptive TX coalescing for now and rely on static
> configuration, we can always add it back later.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/broadcom/bcmsysport.c | 61 ++++--------------------------
> drivers/net/ethernet/broadcom/bcmsysport.h | 1 -
> 2 files changed, 8 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 4e26f606a7f2..1e52bb7d822e 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -15,7 +15,6 @@
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/netdevice.h>
> -#include <linux/net_dim.h>
> #include <linux/etherdevice.h>
> #include <linux/platform_device.h>
> #include <linux/of.h>
> @@ -588,7 +587,8 @@ static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
> rdma_writel(priv, reg, RDMA_MBDONE_INTR);
> }
>
> -static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
> +static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring,
> + struct ethtool_coalesce *ec)
> {
> struct bcm_sysport_priv *priv = ring->priv;
> u32 reg;
> @@ -596,8 +596,8 @@ static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring *ring)
> reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(ring->index));
> reg &= ~(RING_INTR_THRESH_MASK |
> RING_TIMEOUT_MASK << RING_TIMEOUT_SHIFT);
> - reg |= ring->dim.coal_pkts;
> - reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192) <<
> + reg |= ec->tx_max_coalesced_frames;
> + reg |= DIV_ROUND_UP(ec->tx_coalesce_usecs * 1000, 8192) <<
> RING_TIMEOUT_SHIFT;
> tdma_writel(priv, reg, TDMA_DESC_RING_INTR_CONTROL(ring->index));
> }
> @@ -606,18 +606,12 @@ static int bcm_sysport_get_coalesce(struct net_device *dev,
> struct ethtool_coalesce *ec)
> {
> struct bcm_sysport_priv *priv = netdev_priv(dev);
> - struct bcm_sysport_tx_ring *ring;
> - unsigned int i;
> u32 reg;
>
> reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(0));
>
> ec->tx_coalesce_usecs = (reg >> RING_TIMEOUT_SHIFT) * 8192 / 1000;
> ec->tx_max_coalesced_frames = reg & RING_INTR_THRESH_MASK;
> - for (i = 0; i < dev->num_tx_queues; i++) {
> - ring = &priv->tx_rings[i];
> - ec->use_adaptive_tx_coalesce |= ring->dim.use_dim;
> - }
>
> reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>
> @@ -632,7 +626,6 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
> struct ethtool_coalesce *ec)
> {
> struct bcm_sysport_priv *priv = netdev_priv(dev);
> - struct bcm_sysport_tx_ring *ring;
> unsigned int i;
>
> /* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -646,20 +639,12 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
> return -EINVAL;
>
> if ((ec->tx_coalesce_usecs == 0 && ec->tx_max_coalesced_frames == 0) ||
> - (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0))
> + (ec->rx_coalesce_usecs == 0 && ec->rx_max_coalesced_frames == 0) ||
> + ec->use_adaptive_tx_coalesce)
> return -EINVAL;
>
> - for (i = 0; i < dev->num_tx_queues; i++) {
> - ring = &priv->tx_rings[i];
> - ring->dim.coal_pkts = ec->tx_max_coalesced_frames;
> - ring->dim.coal_usecs = ec->tx_coalesce_usecs;
> - if (!ec->use_adaptive_tx_coalesce && ring->dim.use_dim) {
> - ring->dim.coal_pkts = 1;
> - ring->dim.coal_usecs = 0;
> - }
> - ring->dim.use_dim = ec->use_adaptive_tx_coalesce;
> - bcm_sysport_set_tx_coalesce(ring);
> - }
> + for (i = 0; i < dev->num_tx_queues; i++)
> + bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>
> priv->dim.coal_usecs = ec->rx_coalesce_usecs;
> priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
> @@ -940,8 +925,6 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
> ring->packets += pkts_compl;
> ring->bytes += bytes_compl;
> u64_stats_update_end(&priv->syncp);
> - ring->dim.packets = pkts_compl;
> - ring->dim.bytes = bytes_compl;
>
> ring->c_index = c_index;
>
> @@ -987,7 +970,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
> {
> struct bcm_sysport_tx_ring *ring =
> container_of(napi, struct bcm_sysport_tx_ring, napi);
> - struct net_dim_sample dim_sample;
> unsigned int work_done = 0;
>
> work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
> @@ -1004,12 +986,6 @@ static int bcm_sysport_tx_poll(struct napi_struct *napi, int budget)
> return 0;
> }
>
> - if (ring->dim.use_dim) {
> - net_dim_sample(ring->dim.event_ctr, ring->dim.packets,
> - ring->dim.bytes, &dim_sample);
> - net_dim(&ring->dim.dim, dim_sample);
> - }
> -
> return budget;
> }
>
> @@ -1089,23 +1065,6 @@ static void bcm_sysport_dim_work(struct work_struct *work)
> dim->state = NET_DIM_START_MEASURE;
> }
>
> -static void bcm_sysport_dim_tx_work(struct work_struct *work)
> -{
> - struct net_dim *dim = container_of(work, struct net_dim, work);
> - struct bcm_sysport_net_dim *ndim =
> - container_of(dim, struct bcm_sysport_net_dim, dim);
> - struct bcm_sysport_tx_ring *ring =
> - container_of(ndim, struct bcm_sysport_tx_ring, dim);
> - struct net_dim_cq_moder cur_profile =
> - net_dim_get_profile(dim->mode, dim->profile_ix);
> -
> - ring->dim.coal_usecs = cur_profile.usec;
> - ring->dim.coal_pkts = cur_profile.pkts;
> -
> - bcm_sysport_set_tx_coalesce(ring);
> - dim->state = NET_DIM_START_MEASURE;
> -}
> -
> /* RX and misc interrupt routine */
> static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
> {
> @@ -1152,7 +1111,6 @@ static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
> continue;
>
> txr = &priv->tx_rings[ring];
> - txr->dim.event_ctr++;
>
> if (likely(napi_schedule_prep(&txr->napi))) {
> intrl2_0_mask_set(priv, ring_bit);
> @@ -1185,7 +1143,6 @@ static irqreturn_t bcm_sysport_tx_isr(int irq, void *dev_id)
> continue;
>
> txr = &priv->tx_rings[ring];
> - txr->dim.event_ctr++;
>
> if (likely(napi_schedule_prep(&txr->napi))) {
> intrl2_1_mask_set(priv, BIT(ring));
> @@ -1551,7 +1508,6 @@ static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
> reg |= (1 << index);
> tdma_writel(priv, reg, TDMA_TIER1_ARB_0_QUEUE_EN);
>
> - bcm_sysport_init_dim(&ring->dim, bcm_sysport_dim_tx_work);
> napi_enable(&ring->napi);
>
> netif_dbg(priv, hw, priv->netdev,
> @@ -1582,7 +1538,6 @@ static void bcm_sysport_fini_tx_ring(struct bcm_sysport_priv *priv,
> return;
>
> napi_disable(&ring->napi);
> - cancel_work_sync(&ring->dim.dim.work);
> netif_napi_del(&ring->napi);
>
> bcm_sysport_tx_clean(priv, ring);
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index e1c97d4a82b4..57e18ef8f206 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -723,7 +723,6 @@ struct bcm_sysport_tx_ring {
> struct bcm_sysport_priv *priv; /* private context backpointer */
> unsigned long packets; /* packets statistics */
> unsigned long bytes; /* bytes statistics */
> - struct bcm_sysport_net_dim dim; /* Net DIM context */
> unsigned int switch_queue; /* switch port queue number */
> unsigned int switch_port; /* switch port queue number */
> bool inspect; /* inspect switch port and queue */
>
Reviewed-by: Tal Gilboa <talgi@mellanox.com>
^ permalink raw reply
* [PATCH iproute2-next 1/1] tc: enable json output for actions
From: Roman Mashak @ 2018-03-28 20:59 UTC (permalink / raw)
To: dsahern; +Cc: netdev, stephen, kernel, jhs, xiyou.wangcong, jiri, Roman Mashak
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
---
tc/m_action.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/tc/m_action.c b/tc/m_action.c
index 6c3049c7db88..8891659ae15a 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -364,6 +364,7 @@ tc_print_action(FILE *f, const struct rtattr *arg, unsigned short tot_acts)
if (tab_flush && NULL != tb[0] && NULL == tb[1])
return tc_print_action_flush(f, tb[0]);
+ open_json_object(NULL);
open_json_array(PRINT_JSON, "actions");
for (i = 0; i <= tot_acts; i++) {
if (tb[i]) {
@@ -379,6 +380,7 @@ tc_print_action(FILE *f, const struct rtattr *arg, unsigned short tot_acts)
}
close_json_array(PRINT_JSON, NULL);
+ close_json_object();
return 0;
}
@@ -405,7 +407,10 @@ int print_action(const struct sockaddr_nl *who,
if (tb[TCA_ROOT_COUNT])
tot_acts = RTA_DATA(tb[TCA_ROOT_COUNT]);
- fprintf(fp, "total acts %d\n", tot_acts ? *tot_acts:0);
+ open_json_object(NULL);
+ print_uint(PRINT_ANY, "total acts", "total acts %u",
+ tot_acts ? *tot_acts : 0);
+ close_json_object();
if (tb[TCA_ACT_TAB] == NULL) {
if (n->nlmsg_type != RTM_GETACTION)
fprintf(stderr, "print_action: NULL kind\n");
@@ -531,10 +536,16 @@ static int tc_action_gd(int cmd, unsigned int flags,
return 1;
}
- if (cmd == RTM_GETACTION && print_action(NULL, ans, stdout) < 0) {
- fprintf(stderr, "Dump terminated\n");
- free(ans);
- return 1;
+ if (cmd == RTM_GETACTION) {
+ new_json_obj(json);
+ ret = print_action(NULL, ans, stdout);
+ if (ret < 0) {
+ fprintf(stderr, "Dump terminated\n");
+ free(ans);
+ delete_json_obj();
+ return 1;
+ }
+ delete_json_obj();
}
free(ans);
@@ -675,7 +686,9 @@ static int tc_act_list_or_flush(int *argc_p, char ***argv_p, int event)
perror("Cannot send dump request");
return 1;
}
+ new_json_obj(json);
ret = rtnl_dump_filter(&rth, print_action, stdout);
+ delete_json_obj();
}
if (event == RTM_DELACTION) {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] sunrpc: remove incorrect HMAC request initialization
From: J . Bruce Fields @ 2018-03-28 21:12 UTC (permalink / raw)
To: Eric Biggers
Cc: Trond Myklebust, Anna Schumaker, Jeff Layton, linux-nfs, netdev,
linux-crypto, Herbert Xu, Michael Young, stable
In-Reply-To: <20180328175722.193355-1-ebiggers@google.com>
Applying, thanks!--b.
On Wed, Mar 28, 2018 at 10:57:22AM -0700, Eric Biggers wrote:
> make_checksum_hmac_md5() is allocating an HMAC transform and doing
> crypto API calls in the following order:
>
> crypto_ahash_init()
> crypto_ahash_setkey()
> crypto_ahash_digest()
>
> This is wrong because it makes no sense to init() the request before a
> key has been set, given that the initial state depends on the key. And
> digest() is short for init() + update() + final(), so in this case
> there's no need to explicitly call init() at all.
>
> Before commit 9fa68f620041 ("crypto: hash - prevent using keyed hashes
> without setting key") the extra init() had no real effect, at least for
> the software HMAC implementation. (There are also hardware drivers that
> implement HMAC-MD5, and it's not immediately obvious how gracefully they
> handle init() before setkey().) But now the crypto API detects this
> incorrect initialization and returns -ENOKEY. This is breaking NFS
> mounts in some cases.
>
> Fix it by removing the incorrect call to crypto_ahash_init().
>
> Reported-by: Michael Young <m.a.young@durham.ac.uk>
> Fixes: 9fa68f620041 ("crypto: hash - prevent using keyed hashes without setting key")
> Fixes: fffdaef2eb4a ("gss_krb5: Add support for rc4-hmac encryption")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index 12649c9fedab..8654494b4d0a 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
>
> ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
>
> - err = crypto_ahash_init(req);
> - if (err)
> - goto out;
> err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
> if (err)
> goto out;
> --
> 2.17.0.rc1.321.gba9d0f2565-goog
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: bcmgenet: Fix coalescing settings handling
From: Tal Gilboa @ 2018-03-28 21:23 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, Michael Chan, gospo,
saeedm
In-Reply-To: <20180327194707.31857-4-f.fainelli@gmail.com>
On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> There were a number of issues with setting the RX coalescing parameters:
>
> - we would not be preserving values that would have been configured
> across close/open calls, instead we would always reset to no timeout
> and 1 interrupt per packet, this would also prevent DIM from setting its
> default usec/pkts values
>
> - when adaptive RX would be turned on, we woud not be fetching the
> default parameters, we would stay with no timeout/1 packet per interrupt
> until the estimator kicks in and changes that
>
> - finally disabling adaptive RX coalescing while providing parameters
> would not be honored, and we would stay with whatever DIM had previously
> determined instead of the user requested parameters
>
> Fixes: 9f4ca05827a2 ("net: bcmgenet: Add support for adaptive RX coalescing")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/broadcom/genet/bcmgenet.c | 78 ++++++++++++++++++--------
> drivers/net/ethernet/broadcom/genet/bcmgenet.h | 4 +-
> 2 files changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index 7db8edc643ec..76409debb796 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -625,18 +625,18 @@ static int bcmgenet_get_coalesce(struct net_device *dev,
> return 0;
> }
>
> -static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring)
> +static void bcmgenet_set_rx_coalesce(struct bcmgenet_rx_ring *ring,
> + u32 usecs, u32 pkts)
> {
> struct bcmgenet_priv *priv = ring->priv;
> unsigned int i = ring->index;
> u32 reg;
>
> - bcmgenet_rdma_ring_writel(priv, i, ring->dim.coal_pkts,
> - DMA_MBUF_DONE_THRESH);
> + bcmgenet_rdma_ring_writel(priv, i, pkts, DMA_MBUF_DONE_THRESH);
>
> reg = bcmgenet_rdma_readl(priv, DMA_RING0_TIMEOUT + i);
> reg &= ~DMA_TIMEOUT_MASK;
> - reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192);
> + reg |= DIV_ROUND_UP(usecs * 1000, 8192);
> bcmgenet_rdma_writel(priv, reg, DMA_RING0_TIMEOUT + i);
> }
>
> @@ -645,6 +645,8 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
> {
> struct bcmgenet_priv *priv = netdev_priv(dev);
> struct bcmgenet_rx_ring *ring;
> + struct net_dim_cq_moder moder;
> + u32 usecs, pkts;
> unsigned int i;
>
> /* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -682,25 +684,37 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
>
> for (i = 0; i < priv->hw_params->rx_queues; i++) {
> ring = &priv->rx_rings[i];
> - ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> - ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> - if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> - ring->dim.coal_pkts = 1;
> - ring->dim.coal_usecs = 0;
> +
> + ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> + ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> + usecs = ring->rx_coalesce_usecs;
> + pkts = ring->rx_max_coalesced_frames;
> +
> + if (ec->use_adaptive_rx_coalesce) {
> + moder = net_dim_get_def_profile(ring->dim.dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> }
What if DIM is already in use? We don't want to set default values if
DIM is already working.
> +
> ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> - bcmgenet_set_rx_coalesce(ring);
> + bcmgenet_set_rx_coalesce(ring, usecs, pkts);
> }
>
> ring = &priv->rx_rings[DESC_INDEX];
> - ring->dim.coal_usecs = ec->rx_coalesce_usecs;
> - ring->dim.coal_pkts = ec->rx_max_coalesced_frames;
> - if (!ec->use_adaptive_rx_coalesce && ring->dim.use_dim) {
> - ring->dim.coal_pkts = 1;
> - ring->dim.coal_usecs = 0;
> +
> + ring->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> + ring->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> + usecs = ring->rx_coalesce_usecs;
> + pkts = ring->rx_max_coalesced_frames;
> +
> + if (ec->use_adaptive_rx_coalesce) {
> + moder = net_dim_get_def_profile(ring->dim.dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> }
Same as above.
> +
> ring->dim.use_dim = ec->use_adaptive_rx_coalesce;
> - bcmgenet_set_rx_coalesce(ring);
> + bcmgenet_set_rx_coalesce(ring, usecs, pkts);
>
> return 0;
> }
> @@ -1924,10 +1938,7 @@ static void bcmgenet_dim_work(struct work_struct *work)
> struct net_dim_cq_moder cur_profile =
> net_dim_get_profile(dim->mode, dim->profile_ix);
>
> - ring->dim.coal_usecs = cur_profile.usec;
> - ring->dim.coal_pkts = cur_profile.pkts;
> -
> - bcmgenet_set_rx_coalesce(ring);
> + bcmgenet_set_rx_coalesce(ring, cur_profile.usec, cur_profile.pkts);
> dim->state = NET_DIM_START_MEASURE;
> }
>
> @@ -2079,14 +2090,30 @@ static void init_umac(struct bcmgenet_priv *priv)
> dev_dbg(kdev, "done init umac\n");
> }
>
> -static void bcmgenet_init_dim(struct bcmgenet_net_dim *dim,
> +static void bcmgenet_init_dim(struct bcmgenet_rx_ring *ring,
> void (*cb)(struct work_struct *work))
> {
> + struct bcmgenet_net_dim *dim = &ring->dim;
> + struct net_dim_cq_moder moder;
> + u32 usecs, pkts;
> +
> INIT_WORK(&dim->dim.work, cb);
> dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
> dim->event_ctr = 0;
> dim->packets = 0;
> dim->bytes = 0;
I would expect only dim related code in a function called init_dim(). I
think the code below should be elsewhere. Or maybe change the function
name to init_rx_coalesce().
> +
> + usecs = ring->rx_coalesce_usecs;
> + pkts = ring->rx_max_coalesced_frames;
> +
> + /* If DIM was enabled, re-apply default parameters */
> + if (dim->use_dim) {
> + moder = net_dim_get_def_profile(dim->dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> + }
> +
> + bcmgenet_set_rx_coalesce(ring, usecs, pkts);
> }
>
> /* Initialize a Tx ring along with corresponding hardware registers */
> @@ -2178,7 +2205,7 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
> if (ret)
> return ret;
>
> - bcmgenet_init_dim(&ring->dim, bcmgenet_dim_work);
> + bcmgenet_init_dim(ring, bcmgenet_dim_work);
>
> /* Initialize Rx NAPI */
> netif_napi_add(priv->dev, &ring->napi, bcmgenet_rx_poll,
> @@ -2186,7 +2213,6 @@ static int bcmgenet_init_rx_ring(struct bcmgenet_priv *priv,
>
> bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_PROD_INDEX);
> bcmgenet_rdma_ring_writel(priv, index, 0, RDMA_CONS_INDEX);
> - bcmgenet_rdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH);
> bcmgenet_rdma_ring_writel(priv, index,
> ((size << DMA_RING_SIZE_SHIFT) |
> RX_BUF_LENGTH), DMA_RING_BUF_SIZE);
> @@ -3424,6 +3450,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
> struct net_device *dev;
> const void *macaddr;
> struct resource *r;
> + unsigned int i;
> int err = -EIO;
> const char *phy_mode_str;
>
> @@ -3552,6 +3579,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
> netif_set_real_num_tx_queues(priv->dev, priv->hw_params->tx_queues + 1);
> netif_set_real_num_rx_queues(priv->dev, priv->hw_params->rx_queues + 1);
>
> + /* Set default coalescing parameters */
> + for (i = 0; i < priv->hw_params->rx_queues; i++)
> + priv->rx_rings[i].rx_max_coalesced_frames = 1;
> + priv->rx_rings[DESC_INDEX].rx_max_coalesced_frames = 1;
> +
> /* libphy will determine the link state */
> netif_carrier_off(dev);
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 22c41e0430fb..b773bc07edf7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -578,8 +578,6 @@ struct bcmgenet_net_dim {
> u16 event_ctr;
> unsigned long packets;
> unsigned long bytes;
> - u32 coal_usecs;
> - u32 coal_pkts;
> struct net_dim dim;
> };
>
> @@ -598,6 +596,8 @@ struct bcmgenet_rx_ring {
> unsigned int end_ptr; /* Rx ring end CB ptr */
> unsigned int old_discards;
> struct bcmgenet_net_dim dim;
> + u32 rx_max_coalesced_frames;
> + u32 rx_coalesce_usecs;
> void (*int_enable)(struct bcmgenet_rx_ring *);
> void (*int_disable)(struct bcmgenet_rx_ring *);
> struct bcmgenet_priv *priv;
>
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: systemport: Fix coalescing settings handling
From: Tal Gilboa @ 2018-03-28 21:23 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, Michael Chan, gospo,
saeedm
In-Reply-To: <20180327194707.31857-3-f.fainelli@gmail.com>
On 3/27/2018 10:47 PM, Florian Fainelli wrote:
> There were a number of issues with setting the RX coalescing parameters:
>
> - we would not be preserving values that would have been configured
> across close/open calls, instead we would always reset to no timeout
> and 1 interrupt per packet, this would also prevent DIM from setting its
> default usec/pkts values
>
> - when adaptive RX would be turned on, we woud not be fetching the
> default parameters, we would stay with no timeout/1 packet per
> interrupt until the estimator kicks in and changes that
>
> - finally disabling adaptive RX coalescing while providing parameters
> would not be honored, and we would stay with whatever DIM had
> previously determined instead of the user requested parameters
>
> Fixes: b6e0e875421e ("net: systemport: Implement adaptive interrupt coalescing")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/net/ethernet/broadcom/bcmsysport.c | 57 ++++++++++++++++++++----------
> drivers/net/ethernet/broadcom/bcmsysport.h | 4 +--
> 2 files changed, 41 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 1e52bb7d822e..43ad6300c351 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -574,16 +574,16 @@ static int bcm_sysport_set_wol(struct net_device *dev,
> return 0;
> }
>
> -static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv,
> + u32 usecs, u32 pkts)
> {
> u32 reg;
>
> reg = rdma_readl(priv, RDMA_MBDONE_INTR);
> reg &= ~(RDMA_INTR_THRESH_MASK |
> RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
> - reg |= priv->dim.coal_pkts;
> - reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
> - RDMA_TIMEOUT_SHIFT;
> + reg |= pkts;
> + reg |= DIV_ROUND_UP(usecs * 1000, 8192) << RDMA_TIMEOUT_SHIFT;
> rdma_writel(priv, reg, RDMA_MBDONE_INTR);
> }
>
> @@ -626,6 +626,8 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
> struct ethtool_coalesce *ec)
> {
> struct bcm_sysport_priv *priv = netdev_priv(dev);
> + struct net_dim_cq_moder moder;
> + u32 usecs, pkts;
> unsigned int i;
>
> /* Base system clock is 125Mhz, DMA timeout is this reference clock
> @@ -646,15 +648,22 @@ static int bcm_sysport_set_coalesce(struct net_device *dev,
> for (i = 0; i < dev->num_tx_queues; i++)
> bcm_sysport_set_tx_coalesce(&priv->tx_rings[i], ec);
>
> - priv->dim.coal_usecs = ec->rx_coalesce_usecs;
> - priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
> + priv->rx_coalesce_usecs = ec->rx_coalesce_usecs;
> + priv->rx_max_coalesced_frames = ec->rx_max_coalesced_frames;
> + usecs = priv->rx_coalesce_usecs;
> + pkts = priv->rx_max_coalesced_frames;
>
> - if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
> - priv->dim.coal_pkts = 1;
> - priv->dim.coal_usecs = 0;
> + /* If DIM is enabled, immediately obtain default parameters */
> + if (ec->use_adaptive_rx_coalesce) {
> + moder = net_dim_get_def_profile(priv->dim.dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> }
What if DIM is already in use? We don't want to set default values if
DIM is already working.
> +
> priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
> - bcm_sysport_set_rx_coalesce(priv);
> +
> + /* Apply desired coalescing parameters */
> + bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
>
> return 0;
> }
> @@ -1058,10 +1067,7 @@ static void bcm_sysport_dim_work(struct work_struct *work)
> struct net_dim_cq_moder cur_profile =
> net_dim_get_profile(dim->mode, dim->profile_ix);
>
> - priv->dim.coal_usecs = cur_profile.usec;
> - priv->dim.coal_pkts = cur_profile.pkts;
> -
> - bcm_sysport_set_rx_coalesce(priv);
> + bcm_sysport_set_rx_coalesce(priv, cur_profile.usec, cur_profile.pkts);
> dim->state = NET_DIM_START_MEASURE;
> }
>
> @@ -1408,14 +1414,30 @@ static void bcm_sysport_adj_link(struct net_device *dev)
> phy_print_status(phydev);
> }
>
> -static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
> +static void bcm_sysport_init_dim(struct bcm_sysport_priv *priv,
> void (*cb)(struct work_struct *work))
> {
> + struct bcm_sysport_net_dim *dim = &priv->dim;
> + struct net_dim_cq_moder moder;
> + u32 usecs, pkts;
> +
> INIT_WORK(&dim->dim.work, cb);
> dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
> dim->event_ctr = 0;
> dim->packets = 0;
> dim->bytes = 0;
I would expect only dim related code in a function called init_dim(). I
think the code below should be elsewhere. Or maybe change the function
name to init_rx_coalesce().
> +
> + usecs = priv->rx_coalesce_usecs;
> + pkts = priv->rx_max_coalesced_frames;
> +
> + /* If DIM was enabled, re-apply default parameters */
> + if (dim->use_dim) {
> + moder = net_dim_get_def_profile(dim->dim.mode);
> + usecs = moder.usec;
> + pkts = moder.pkts;
> + }
> +
> + bcm_sysport_set_rx_coalesce(priv, usecs, pkts);
> }
>
> static int bcm_sysport_init_tx_ring(struct bcm_sysport_priv *priv,
> @@ -1658,8 +1680,6 @@ static int bcm_sysport_init_rx_ring(struct bcm_sysport_priv *priv)
> rdma_writel(priv, 0, RDMA_END_ADDR_HI);
> rdma_writel(priv, priv->num_rx_desc_words - 1, RDMA_END_ADDR_LO);
>
> - rdma_writel(priv, 1, RDMA_MBDONE_INTR);
> -
> netif_dbg(priv, hw, priv->netdev,
> "RDMA cfg, num_rx_bds=%d, rx_bds=%p\n",
> priv->num_rx_bds, priv->rx_bds);
> @@ -1827,7 +1847,7 @@ static void bcm_sysport_netif_start(struct net_device *dev)
> struct bcm_sysport_priv *priv = netdev_priv(dev);
>
> /* Enable NAPI */
> - bcm_sysport_init_dim(&priv->dim, bcm_sysport_dim_work);
> + bcm_sysport_init_dim(priv, bcm_sysport_dim_work);
> napi_enable(&priv->napi);
>
> /* Enable RX interrupt and TX ring full interrupt */
> @@ -2333,6 +2353,7 @@ static int bcm_sysport_probe(struct platform_device *pdev)
> /* libphy will adjust the link state accordingly */
> netif_carrier_off(dev);
>
> + priv->rx_max_coalesced_frames = 1;
> u64_stats_init(&priv->syncp);
>
> priv->dsa_notifier.notifier_call = bcm_sysport_dsa_notifier;
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index 57e18ef8f206..d6e5d0cbf3a3 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -701,8 +701,6 @@ struct bcm_sysport_net_dim {
> u16 event_ctr;
> unsigned long packets;
> unsigned long bytes;
> - u32 coal_usecs;
> - u32 coal_pkts;
> struct net_dim dim;
> };
>
> @@ -755,6 +753,8 @@ struct bcm_sysport_priv {
> unsigned int rx_c_index;
>
> struct bcm_sysport_net_dim dim;
> + u32 rx_max_coalesced_frames;
> + u32 rx_coalesce_usecs;
>
> /* PHY device */
> struct device_node *phy_dn;
>
^ permalink raw reply
* Re: [PATCH 5/6] rhashtable: support guaranteed successful insertion.
From: NeilBrown @ 2018-03-28 21:26 UTC (permalink / raw)
To: Herbert Xu; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180328072727.GA17306@gondor.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 3887 bytes --]
On Wed, Mar 28 2018, Herbert Xu wrote:
> On Wed, Mar 28, 2018 at 06:04:40PM +1100, NeilBrown wrote:
>>
>> I disagree. My patch 6 only makes it common instead of exceedingly
>> rare. If any table in the list other than the first has a chain with 16
>> elements, then trying to insert an element with a hash which matches
>> that chain will fail with -EBUSY. This is theoretically possible
>> already, though astronomically unlikely. So that case will never be
>> tested for.
>
> No that's not true. If the table is correctly sized then the
> probability of having a chain with 16 elements is extremely low.
I say "astronomically unlikely", you say "probability .. is extremely
low". I think we are in agreement here.
The point remains that if an error *can* be returned then I have to
write code to handle it and test that code. I'd rather not.
>
> Even if it does happen we won't fail because we will perform
> an immediate rehash. We only fail if it happens right away
> after the rehash (that is, at least another 16 elements have
> been inserted and you're trying to insert a 17th element, all
> while the new hash table has not been completely populated),
> which means that somebody has figured out our hash secret and
> failing in that case makes sense.
>
>> It is hard to know if it is necessary. And making the new table larger
>> will make the error less likely, but still won't make it impossible. So
>> callers will have to handle it - just like they currently have to handle
>> -ENOMEM even though it is highly unlikely (and not strictly necessary).
>
> Callers should not handle an ENOMEM error by retrying. Nor should
> they retry an EBUSY return value.
I never suggested retrying, but I would have to handle it somehow. I'd
rather not.
>
>> Are these errors ever actually useful? I thought I had convinced myself
>> before that they were (to throttle attacks on the hash function), but
>> they happen even less often than I thought.
>
> The EBUSY error indicates that the hash table has essentially
> degenereated into a linked list because somebody has worked out
> our hash secret.
While I have no doubt that there are hashtables where someone could try
to attack the hash, I am quite sure there are others where is such an
attack is meaningless - any code which could generate the required range of
keys, could do far worse things more easily.
>
>> Maybe. Reading a percpu counter isn't cheap. Reading it whenever a hash
>> chain reaches 16 is reasonable, but I think we would want to read it a
>> lot more often than that. So probably store the last-sampled time (with
>> no locking) and only sample the counter if last-sampled is more than
>> jiffies - 10*HZ (???)
>
> We could also take the spinlock table approach and have a counter
> per bucket spinlock. This should be sufficient as you'll contend
> on the bucket spinlock table anyway.
Yes, storing a sharded count in the spinlock table does seem like an
appropriate granularity. However that leads me to ask: why do we have
the spinlock table? Why not bit spinlocks in the hashchain head like
include/linux/list_bl uses?
>
> This also allows us to estimate the total table size and not have
> to always do a last-ditch growth when it's too late.
I don't understand how it can ever be "too late", though I appreciate
that in some cases "sooner" is better than "later"
If we give up on the single atomic_t counter, then we must accept that
the number of elements could exceed any given value. The only promise
we can provide is that it wont exceed N% of the table size for more than
T seconds.
Thanks,
NeilBrown
>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [pci PATCH v7 1/5] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
From: Rustad, Mark D @ 2018-03-28 21:30 UTC (permalink / raw)
To: Alexander Duyck
Cc: bhelgaas@google.com, Duyck, Alexander H,
linux-pci@vger.kernel.org, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, Netdev, Daly, Dan, LKML,
linux-nvme@lists.infradead.org, Busch, Keith, netanel@amazon.com,
ddutile@redhat.com, mheyne@amazon.de, Wang, Liang-min,
dwmw2@infradead.org, hch@lst.de, dwmw@amazon.co.uk
In-Reply-To: <20180315184055.3102.2435.stgit@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]
On Mar 15, 2018, at 11:41 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> This patch adds a common configuration function called
> pci_sriov_configure_simple that will allow for managing VFs on devices
> where the PF is not capable of managing VF resources.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>
> v5: New patch replacing pci_sriov_configure_unmanaged with
> pci_sriov_configure_simple
> Dropped bits related to autoprobe changes
> v6: Defined pci_sriov_configure_simple as NULL if IOV is disabled
> v7: Updated pci_sriov_configure_simple to drop need for err value
> Fixed comment explaining why pci_sriov_configure_simple is NULL
>
> drivers/pci/iov.c | 31 +++++++++++++++++++++++++++++++
> include/linux/pci.h | 3 +++
> 2 files changed, 34 insertions(+)
Tested with the device identified in patch #2.
Tested-by: Mark Rustad <mark.d.rustad@intel.com>
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [pci PATCH v7 2/5] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices
From: Rustad, Mark D @ 2018-03-28 21:31 UTC (permalink / raw)
To: Alexander Duyck
Cc: bhelgaas@google.com, Duyck, Alexander H,
linux-pci@vger.kernel.org, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, netdev@vger.kernel.org, Daly, Dan,
linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
Busch, Keith, netanel@amazon.com, ddutile@redhat.com,
mheyne@amazon.de, Wang, Liang-min, dwmw2@infradead.org,
hch@lst.de, "dwmw@amaz
In-Reply-To: <20180315184132.3102.90947.stgit@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]
On Mar 15, 2018, at 11:42 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> Hardware-realized virtio_pci devices can implement SR-IOV, so this
> patch enables its use. The device in question is an upcoming Intel
> NIC that implements both a virtio_net PF and virtio_net VFs. These
> are hardware realizations of what has been up to now been a software
> interface.
>
> The device in question has the following 4-part PCI IDs:
>
> PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
> VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe
>
> The patch currently needs no check for device ID, because the callback
> will never be made for devices that do not assert the capability or
> when run on a platform incapable of SR-IOV.
>
> One reason for this patch is because the hardware requires the
> vendor ID of a VF to be the same as the vendor ID of the PF that
> created it. So it seemed logical to simply have a fully-functioning
> virtio_net PF create the VFs. This patch makes that possible.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>
> v4: Dropped call to pci_disable_sriov in virtio_pci_remove function
> v5: Replaced call to pci_sriov_configure_unmanaged with
> pci_sriov_configure_simple
> v6: Dropped "#ifdef" checks for IOV wrapping sriov_configure definition
> v7: No code change, added Reviewed-by
>
> drivers/virtio/virtio_pci_common.c | 1 +
> 1 file changed, 1 insertion(+)
Tested with the identified device.
Tested-by: Mark Rustad <mark.d.rustad@intel.com>
--
Mark Rustad, Networking Division, Intel Corporation
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 21:31 UTC (permalink / raw)
To: Nicholas Piggin, David Miller
Cc: paulmck, arnd, linux-rdma, linuxppc-dev, linus971, will.deacon,
alexander.duyck, okaya, jgg, David.Laight, oohall, netdev,
alexander.h.duyck, torvalds
In-Reply-To: <20180329022324.037c3f39@roar.ozlabs.ibm.com>
On Thu, 2018-03-29 at 02:23 +1000, Nicholas Piggin wrote:
> On Wed, 28 Mar 2018 11:55:09 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
>
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Thu, 29 Mar 2018 02:13:16 +1100
> >
> > > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > > the archs are unused or dead anyway.
> >
> > Agreed.
>
> While we're making decrees here, can we do something about mmiowb?
> The semantics are basically indecipherable.
I was going to tackle that next :-)
> This is a variation on the mandatory write barrier that causes writes to weakly
> ordered I/O regions to be partially ordered. Its effects may go beyond the
> CPU->Hardware interface and actually affect the hardware at some level.
>
> How can a driver writer possibly get that right?
>
> IIRC it was added for some big ia64 system that was really expensive
> to implement the proper wmb() semantics on. So wmb() semantics were
> quietly downgraded, then the subsequently broken drivers they cared
> about were fixed by adding the stronger mmiowb().
>
> What should have happened was wmb and writel remained correct, sane, and
> expensive, and they add an mmio_wmb() to order MMIO stores made by the
> writel_relaxed accessors, then use that to speed up the few drivers they
> care about.
>
> Now that ia64 doesn't matter too much, can we deprecate mmiowb and just
> make wmb ordering talk about stores to the device, not to some
> intermediate stage of the interconnect where it can be subsequently
> reordered wrt the device? Drivers can be converted back to using wmb
> or writel gradually.
I was under the impression that mmiowb was specifically about ordering
writel's with a subsequent spin_unlock, without it, MMIOs from
different CPUs (within the same lock) would still arrive OO.
If that's indeed the case, I would suggest ia64 switches to a similar
per-cpu flag trick powerpc uses.
Cheers,
Ben.
> Thanks,
> Nick
^ permalink raw reply
* Re: [PATCH 4/6] rhashtable: allow a walk of the hash table without missing objects.
From: NeilBrown @ 2018-03-28 21:34 UTC (permalink / raw)
To: Herbert Xu; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180328073023.GB17306@gondor.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]
On Wed, Mar 28 2018, Herbert Xu wrote:
> On Wed, Mar 28, 2018 at 06:17:57PM +1100, NeilBrown wrote:
>>
>> Sounds like over-kill to me.
>> It might be reasonable to have a CONFIG_DEBUG_RHASHTABLE which enables
>> extra to code to catch misuse, but I don't see the justification for
>> always performing these checks.
>> The DEBUG code could just scan the chain (usually quite short) to see if
>> the given element is present. Of course it might have already been
>> rehashed to the next table, so you would to allow for that possibility -
>> probably check tbl->rehash.
>
> No this is not meant to debug users incorrectly using the cursor.
> This is a replacement of your continue interface by automatically
> validating the cursor.
>
> In fact we can make it even more reliable. We can insert the walker
> right into the bucket chain, that way the walking will always be
> consistent.
>
> The only problem is that we need be able to differentiate between
> a walker, a normal object, and the end of the list. I think it
> should be doable.
Yes, I think that could work. The code to stop over a walker object
during an unlocked search wouldn't be straight forward and would need
careful analysis.
However about storing the hash chains in order by object address?
Then rhashtable_walk_start() can easily find it's place regardless of
whether the old object was still present or not, using <= on the
address.
"Insert" would need to record an insert location and insert there rather
than at the head of the chain.
I might try coding that.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ 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