* Re: [PATCH v2 bpf-next 1/4] bpf: enable program stats
From: Eric Dumazet @ 2019-02-23 18:36 UTC (permalink / raw)
To: Alexei Starovoitov, davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190223174422.663270-2-ast@kernel.org>
On 02/23/2019 09:44 AM, Alexei Starovoitov wrote:
...
>
> -#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
> +DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
> +
> +#define BPF_PROG_RUN(prog, ctx) ({ \
> + u32 ret; \
> + cant_sleep(); \
> + if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
> + struct bpf_prog_stats *stats; \
> + u64 start = sched_clock(); \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + stats = this_cpu_ptr(prog->aux->stats); \
> + u64_stats_update_begin(&stats->syncp); \
> + stats->cnt++; \
> + stats->nsecs += sched_clock() - start; \
> + u64_stats_update_end(&stats->syncp); \
> + } else { \
> + ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
> + } \
> + ret; })
>
It seems a cpu running there could still be interrupted (by an interrupt)
and re-enter this section ?
If yes, u64_stats_update_begin() and u64_stats_update_end() are unsafe (on 32bit arches)
u64_stats_update_{begin|end}() assume proper locking, since they use a simple increment.
But then, even on 64bit arches, the stats->{cnt|nsecs} updates are unsafe ?
^ permalink raw reply
* [PATCH v2 bpf-next 3/4] tools/bpf: sync bpf.h into tools
From: Alexei Starovoitov @ 2019-02-23 17:44 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190223174422.663270-1-ast@kernel.org>
sync bpf.h into tools directory
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/include/uapi/linux/bpf.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..d2cb85d85b39 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 runtime;
+ __u64 runcnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
--
2.20.0
^ permalink raw reply related
* [PATCH v2 bpf-next 4/4] tools/bpftool: recognize bpf_prog_info runtime and runcnt
From: Alexei Starovoitov @ 2019-02-23 17:44 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190223174422.663270-1-ast@kernel.org>
$ bpftool p s
1: kprobe tag a56587d488d216c9 gpl runtime 79786 runcnt 8
loaded_at 2019-02-22T12:22:51-0800 uid 0
xlated 352B not jited memlock 4096B
$ bpftool --json --pretty p s
[{
"id": 1,
"type": "kprobe",
"tag": "a56587d488d216c9",
"gpl_compatible": true,
"run_time_ns": 79786,
"run_cnt": 8,
"loaded_at": 1550866971,
"uid": 0,
"bytes_xlated": 352,
"jited": false,
"bytes_memlock": 4096
}
]
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/bpf/bpftool/Documentation/bpftool-prog.rst | 4 +++-
tools/bpf/bpftool/prog.c | 7 +++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 12bc1e2d4b46..d8b46a8f17e0 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -171,7 +171,7 @@ EXAMPLES
::
- 10: xdp name some_prog tag 005a3d2123620c8b gpl
+ 10: xdp name some_prog tag 005a3d2123620c8b gpl runtime 81632 runcnt 10
loaded_at 2017-09-29T20:11:00+0000 uid 0
xlated 528B jited 370B memlock 4096B map_ids 10
@@ -184,6 +184,8 @@ EXAMPLES
"type": "xdp",
"tag": "005a3d2123620c8b",
"gpl_compatible": true,
+ "run_time_ns": 81632,
+ "run_cnt": 10,
"loaded_at": 1506715860,
"uid": 0,
"bytes_xlated": 528,
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index db978c8d76a8..cdb6bd424340 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -214,6 +214,10 @@ static void print_prog_json(struct bpf_prog_info *info, int fd)
info->tag[4], info->tag[5], info->tag[6], info->tag[7]);
jsonw_bool_field(json_wtr, "gpl_compatible", info->gpl_compatible);
+ if (info->runtime) {
+ jsonw_uint_field(json_wtr, "run_time_ns", info->runtime);
+ jsonw_uint_field(json_wtr, "run_cnt", info->runcnt);
+ }
print_dev_json(info->ifindex, info->netns_dev, info->netns_ino);
@@ -277,6 +281,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd)
fprint_hex(stdout, info->tag, BPF_TAG_SIZE, "");
print_dev_plain(info->ifindex, info->netns_dev, info->netns_ino);
printf("%s", info->gpl_compatible ? " gpl" : "");
+ if (info->runtime)
+ printf(" runtime %lld runcnt %lld",
+ info->runtime, info->runcnt);
printf("\n");
if (info->load_time) {
--
2.20.0
^ permalink raw reply related
* [PATCH v2 bpf-next 2/4] bpf: expose program stats via bpf_prog_info
From: Alexei Starovoitov @ 2019-02-23 17:44 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190223174422.663270-1-ast@kernel.org>
Return bpf program runtime and runcnt via bpf_prog_info
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/uapi/linux/bpf.h | 2 ++
kernel/bpf/syscall.c | 5 +++++
2 files changed, 7 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..d2cb85d85b39 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2813,6 +2813,8 @@ struct bpf_prog_info {
__u32 jited_line_info_rec_size;
__u32 nr_prog_tags;
__aligned_u64 prog_tags;
+ __u64 runtime;
+ __u64 runcnt;
} __attribute__((aligned(8)));
struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 9a8c990c5df4..3405e0a4cf77 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2152,6 +2152,7 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
struct bpf_prog_info __user *uinfo = u64_to_user_ptr(attr->info.info);
struct bpf_prog_info info = {};
u32 info_len = attr->info.info_len;
+ struct bpf_prog_stats stats;
char __user *uinsns;
u32 ulen;
int err;
@@ -2191,6 +2192,10 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
if (err)
return err;
+ bpf_prog_get_stats(prog, &stats);
+ info.runtime = stats.nsecs;
+ info.runcnt = stats.cnt;
+
if (!capable(CAP_SYS_ADMIN)) {
info.jited_prog_len = 0;
info.xlated_prog_len = 0;
--
2.20.0
^ permalink raw reply related
* [PATCH v2 bpf-next 1/4] bpf: enable program stats
From: Alexei Starovoitov @ 2019-02-23 17:44 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
In-Reply-To: <20190223174422.663270-1-ast@kernel.org>
JITed BPF programs are indistinguishable from kernel functions, but unlike
kernel code BPF code can be changed often.
Typical approach of "perf record" + "perf report" profiling and tuning of
kernel code works just as well for BPF programs, but kernel code doesn't
need to be monitored whereas BPF programs do.
Users load and run large amount of BPF programs.
These BPF stats allow tools monitor the usage of BPF on the server.
The monitoring tools will turn sysctl kernel.bpf_stats_enabled
on and off for few seconds to sample average cost of the programs.
Aggregated data over hours and days will provide an insight into cost of BPF
and alarms can trigger in case given program suddenly gets more expensive.
The cost of two sched_clock() per program invocation adds ~20 nsec.
Fast BPF progs (like selftests/bpf/progs/test_pkt_access.c) will slow down
from ~10 nsec to ~30 nsec.
static_key minimizes the cost of the stats collection.
There is no measurable difference before/after this patch
with kernel.bpf_stats_enabled=0
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 9 +++++++++
include/linux/filter.h | 20 +++++++++++++++++++-
kernel/bpf/core.c | 31 +++++++++++++++++++++++++++++--
kernel/bpf/syscall.c | 34 ++++++++++++++++++++++++++++++++--
kernel/bpf/verifier.c | 7 ++++++-
kernel/sysctl.c | 34 ++++++++++++++++++++++++++++++++++
6 files changed, 129 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..a2132e09dc1c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -16,6 +16,7 @@
#include <linux/rbtree_latch.h>
#include <linux/numa.h>
#include <linux/wait.h>
+#include <linux/u64_stats_sync.h>
struct bpf_verifier_env;
struct perf_event;
@@ -340,6 +341,12 @@ enum bpf_cgroup_storage_type {
#define MAX_BPF_CGROUP_STORAGE_TYPE __BPF_CGROUP_STORAGE_MAX
+struct bpf_prog_stats {
+ u64 cnt;
+ u64 nsecs;
+ struct u64_stats_sync syncp;
+};
+
struct bpf_prog_aux {
atomic_t refcnt;
u32 used_map_cnt;
@@ -389,6 +396,7 @@ struct bpf_prog_aux {
* main prog always has linfo_idx == 0
*/
u32 linfo_idx;
+ struct bpf_prog_stats __percpu *stats;
union {
struct work_struct work;
struct rcu_head rcu;
@@ -559,6 +567,7 @@ void bpf_map_area_free(void *base);
void bpf_map_init_from_attr(struct bpf_map *map, union bpf_attr *attr);
extern int sysctl_unprivileged_bpf_disabled;
+extern int sysctl_bpf_stats_enabled;
int bpf_map_new_fd(struct bpf_map *map, int flags);
int bpf_prog_new_fd(struct bpf_prog *prog);
diff --git a/include/linux/filter.h b/include/linux/filter.h
index f32b3eca5a04..7e5e3db11106 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -533,7 +533,24 @@ struct sk_filter {
struct bpf_prog *prog;
};
-#define BPF_PROG_RUN(filter, ctx) ({ cant_sleep(); (*(filter)->bpf_func)(ctx, (filter)->insnsi); })
+DECLARE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+
+#define BPF_PROG_RUN(prog, ctx) ({ \
+ u32 ret; \
+ cant_sleep(); \
+ if (static_branch_unlikely(&bpf_stats_enabled_key)) { \
+ struct bpf_prog_stats *stats; \
+ u64 start = sched_clock(); \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ stats = this_cpu_ptr(prog->aux->stats); \
+ u64_stats_update_begin(&stats->syncp); \
+ stats->cnt++; \
+ stats->nsecs += sched_clock() - start; \
+ u64_stats_update_end(&stats->syncp); \
+ } else { \
+ ret = (*(prog)->bpf_func)(ctx, (prog)->insnsi); \
+ } \
+ ret; })
#define BPF_SKB_CB_LEN QDISC_CB_PRIV_LEN
@@ -764,6 +781,7 @@ void bpf_prog_free_jited_linfo(struct bpf_prog *prog);
void bpf_prog_free_unused_jited_linfo(struct bpf_prog *prog);
struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags);
+struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags);
struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
gfp_t gfp_extra_flags);
void __bpf_prog_free(struct bpf_prog *fp);
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ef88b167959d..1c14c347f3cf 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -78,7 +78,7 @@ void *bpf_internal_load_pointer_neg_helper(const struct sk_buff *skb, int k, uns
return NULL;
}
-struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
+struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flags)
{
gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
struct bpf_prog_aux *aux;
@@ -104,6 +104,26 @@ struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
return fp;
}
+
+struct bpf_prog *bpf_prog_alloc(unsigned int size, gfp_t gfp_extra_flags)
+{
+ gfp_t gfp_flags = GFP_KERNEL | __GFP_ZERO | gfp_extra_flags;
+ struct bpf_prog *prog;
+
+ prog = bpf_prog_alloc_no_stats(size, gfp_extra_flags);
+ if (!prog)
+ return NULL;
+
+ prog->aux->stats = alloc_percpu_gfp(struct bpf_prog_stats, gfp_flags);
+ if (!prog->aux->stats) {
+ kfree(prog->aux);
+ vfree(prog);
+ return NULL;
+ }
+
+ u64_stats_init(&prog->aux->stats->syncp);
+ return prog;
+}
EXPORT_SYMBOL_GPL(bpf_prog_alloc);
int bpf_prog_alloc_jited_linfo(struct bpf_prog *prog)
@@ -231,7 +251,10 @@ struct bpf_prog *bpf_prog_realloc(struct bpf_prog *fp_old, unsigned int size,
void __bpf_prog_free(struct bpf_prog *fp)
{
- kfree(fp->aux);
+ if (fp->aux) {
+ free_percpu(fp->aux->stats);
+ kfree(fp->aux);
+ }
vfree(fp);
}
@@ -2069,6 +2092,10 @@ int __weak skb_copy_bits(const struct sk_buff *skb, int offset, void *to,
return -EFAULT;
}
+DEFINE_STATIC_KEY_FALSE(bpf_stats_enabled_key);
+EXPORT_SYMBOL(bpf_stats_enabled_key);
+int sysctl_bpf_stats_enabled __read_mostly;
+
/* All definitions of tracepoints related to BPF. */
#define CREATE_TRACE_POINTS
#include <linux/bpf_trace.h>
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ec7c552af76b..9a8c990c5df4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1283,24 +1283,54 @@ static int bpf_prog_release(struct inode *inode, struct file *filp)
return 0;
}
+static void bpf_prog_get_stats(const struct bpf_prog *prog,
+ struct bpf_prog_stats *stats)
+{
+ u64 nsecs = 0, cnt = 0;
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ const struct bpf_prog_stats *st;
+ unsigned int start;
+ u64 tnsecs, tcnt;
+
+ st = per_cpu_ptr(prog->aux->stats, cpu);
+ do {
+ start = u64_stats_fetch_begin_irq(&st->syncp);
+ tnsecs = st->nsecs;
+ tcnt = st->cnt;
+ } while (u64_stats_fetch_retry_irq(&st->syncp, start));
+ nsecs += tnsecs;
+ cnt += tcnt;
+ }
+ stats->nsecs = nsecs;
+ stats->cnt = cnt;
+}
+
#ifdef CONFIG_PROC_FS
static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
{
const struct bpf_prog *prog = filp->private_data;
char prog_tag[sizeof(prog->tag) * 2 + 1] = { };
+ struct bpf_prog_stats stats;
+ bpf_prog_get_stats(prog, &stats);
bin2hex(prog_tag, prog->tag, sizeof(prog->tag));
seq_printf(m,
"prog_type:\t%u\n"
"prog_jited:\t%u\n"
"prog_tag:\t%s\n"
"memlock:\t%llu\n"
- "prog_id:\t%u\n",
+ "prog_id:\t%u\n"
+ "runtime:\t%llu\n"
+ "runcnt:\t%llu\n",
prog->type,
prog->jited,
prog_tag,
prog->pages * 1ULL << PAGE_SHIFT,
- prog->aux->id);
+ prog->aux->id,
+ stats.nsecs,
+ stats.cnt);
}
#endif
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1b9496c41383..0e4edd7e3c5f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7320,7 +7320,12 @@ static int jit_subprogs(struct bpf_verifier_env *env)
subprog_end = env->subprog_info[i + 1].start;
len = subprog_end - subprog_start;
- func[i] = bpf_prog_alloc(bpf_prog_size(len), GFP_USER);
+ /* BPF_PROG_RUN doesn't call subprogs directly,
+ * hence main prog stats include the runtime of subprogs.
+ * subprogs don't have IDs and not reachable via prog_get_next_id
+ * func[i]->aux->stats will never be accessed and stays NULL
+ */
+ func[i] = bpf_prog_alloc_no_stats(bpf_prog_size(len), GFP_USER);
if (!func[i])
goto out_free;
memcpy(func[i]->insnsi, &prog->insnsi[subprog_start],
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index ba4d9e85feb8..86e0771352f2 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -224,6 +224,9 @@ static int proc_dostring_coredump(struct ctl_table *table, int write,
#endif
static int proc_dopipe_max_size(struct ctl_table *table, int write,
void __user *buffer, size_t *lenp, loff_t *ppos);
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos);
#ifdef CONFIG_MAGIC_SYSRQ
/* Note: sysrq code uses its own private copy */
@@ -1230,6 +1233,15 @@ static struct ctl_table kern_table[] = {
.extra2 = &one,
},
#endif
+ {
+ .procname = "bpf_stats_enabled",
+ .data = &sysctl_bpf_stats_enabled,
+ .maxlen = sizeof(sysctl_bpf_stats_enabled),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax_bpf_stats,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
#if defined(CONFIG_TREE_RCU) || defined(CONFIG_PREEMPT_RCU)
{
.procname = "panic_on_rcu_stall",
@@ -3260,6 +3272,28 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
#endif /* CONFIG_PROC_SYSCTL */
+static int proc_dointvec_minmax_bpf_stats(struct ctl_table *table, int write,
+ void __user *buffer, size_t *lenp,
+ loff_t *ppos)
+{
+ int ret, bpf_stats = *(int *)table->data;
+ struct ctl_table tmp = *table;
+
+ if (write && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ tmp.data = &bpf_stats;
+ ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
+ if (write && !ret) {
+ *(int *)table->data = bpf_stats;
+ if (bpf_stats)
+ static_branch_enable(&bpf_stats_enabled_key);
+ else
+ static_branch_disable(&bpf_stats_enabled_key);
+ }
+ return ret;
+}
+
/*
* No sense putting this after each symbol definition, twice,
* exception granted :-)
--
2.20.0
^ permalink raw reply related
* [PATCH v2 bpf-next 0/4] bpf: per program stats
From: Alexei Starovoitov @ 2019-02-23 17:44 UTC (permalink / raw)
To: davem; +Cc: daniel, edumazet, netdev, bpf, kernel-team
Introduce per program stats to monitor the usage BPF
v1->v2:
- fixed u64 stats on 32-bit archs. Thanks Eric
- use more verbose run_time_ns in json output as suggested by Andrii
- refactored prog_alloc and clarified behavior of stats in subprogs
Alexei Starovoitov (4):
bpf: enable program stats
bpf: expose program stats via bpf_prog_info
tools/bpf: sync bpf.h into tools
tools/bpftool: recognize bpf_prog_info runtime and runcnt
include/linux/bpf.h | 9 +++++
include/linux/filter.h | 20 +++++++++-
include/uapi/linux/bpf.h | 2 +
kernel/bpf/core.c | 31 ++++++++++++++-
kernel/bpf/syscall.c | 39 ++++++++++++++++++-
kernel/bpf/verifier.c | 7 +++-
kernel/sysctl.c | 34 ++++++++++++++++
.../bpftool/Documentation/bpftool-prog.rst | 4 +-
tools/bpf/bpftool/prog.c | 7 ++++
tools/include/uapi/linux/bpf.h | 2 +
10 files changed, 148 insertions(+), 7 deletions(-)
--
2.20.0
^ permalink raw reply
* kTLS getsockopt TLS_RX support
From: Hayakawa Yutaro @ 2019-02-23 17:29 UTC (permalink / raw)
To: netdev
Hello,
While trying the kTLS, I found out that currently, there is no support for kTLS getsockopt TLS_RX which extracts receive side crypto information from kTLS socket. Since setting crypto information for RX side is supported, I felt wonder why it is not supported.
Is there any particular reason for it?
Regards,
Yutaro
^ permalink raw reply
* RE: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for coalesced packets
From: Haiyang Zhang @ 2019-02-23 17:29 UTC (permalink / raw)
To: Stephen Hemminger, Haiyang Zhang
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org, KY Srinivasan,
Stephen Hemminger, olaf@aepfle.de, vkuznets, davem@davemloft.net,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190223084624.0d0c60a0@shemminger-XPS-13-9360>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Saturday, February 23, 2019 11:46 AM
> To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; sashal@kernel.org; linux-
> hyperv@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for
> coalesced packets
>
> On Fri, 22 Feb 2019 18:25:03 +0000
> Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
>
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > Incoming packets may have IP header checksum verified by the host.
> > They may not have IP header checksum computed after coalescing.
> > This patch re-compute the checksum when necessary, otherwise the
> > packets may be dropped, because Linux network stack always checks it.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> > drivers/net/hyperv/netvsc_drv.c | 22 +++++++++++++++++++---
> > 1 file changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c index 256adbd044f5..cf4897043e83
> > 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -744,6 +744,14 @@ void netvsc_linkstatus_callback(struct net_device
> *net,
> > schedule_delayed_work(&ndev_ctx->dwork, 0); }
> >
> > +static void netvsc_comp_ipcsum(struct sk_buff *skb) {
> > + struct iphdr *iph = (struct iphdr *)skb->data;
>
> Can you use iphdr(skb) here?
This skb is just allocated by netvsc, the skb->network_header is not set yet.
>
> > +
> > + iph->check = 0;
> > + iph->check = ip_fast_csum(iph, iph->ihl); }
> > +
> > static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> > struct netvsc_channel *nvchan)
> { @@ -770,9 +778,17 @@
> > static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> > /* skb is already created with CHECKSUM_NONE */
> > skb_checksum_none_assert(skb);
> >
> > - /*
> > - * In Linux, the IP checksum is always checked.
> > - * Do L4 checksum offload if enabled and present.
> > + /* Incoming packets may have IP header checksum verified by the
> host.
> > + * They may not have IP header checksum computed after coalescing.
> > + * We compute it here if the flags are set, because on Linux, the IP
> > + * checksum is always checked.
> > + */
> > + if (csum_info && csum_info->receive.ip_checksum_value_invalid &&
> > + csum_info->receive.ip_checksum_succeeded &&
> > + skb->protocol == htons(ETH_P_IP))
> > + netvsc_comp_ipcsum(skb);
>
> Does this still handle for coalesced and non-coalesced packets which are
> received with bad IP checksum? My concern is that you are potentially
> correcting the checksum for a packet whose received checksum was bad.
Windows networking team told me that the flags above indicate host side
already verified the checksum. Online doc is here:
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/indicating-coalesced-segments
If the NIC or miniport driver validates the TCP and IPv4 checksums but does not recompute them for the coalesced segment, it must set the TcpChecksumValueInvalid and IpChecksumValueInvalid flags in the NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO structure. Additionally, in this case the NIC or miniport driver may optionally zero out the TCP and IPv4 header checksum values in the segment.
The NIC and miniport driver must always set the IpChecksumSucceeded and TcpChecksumSucceeded flags in the NDIS_TCP_IP_CHECKSUM_NET_BUFFER_LIST_INFO structure before indicating the coalesced segment.
Thanks,
- Haiyang
^ permalink raw reply
* Fw: [Bug 202651] New: Regression in atlantic.ko between 4.20 and 5.00-rc4 for Aquantia NIC 1d6a:87b1
From: Stephen Hemminger @ 2019-02-23 16:50 UTC (permalink / raw)
To: netdev
Begin forwarded message:
Date: Sat, 23 Feb 2019 09:01:29 +0000
From: bugzilla-daemon@bugzilla.kernel.org
To: stephen@networkplumber.org
Subject: [Bug 202651] New: Regression in atlantic.ko between 4.20 and 5.00-rc4 for Aquantia NIC 1d6a:87b1
https://bugzilla.kernel.org/show_bug.cgi?id=202651
Bug ID: 202651
Summary: Regression in atlantic.ko between 4.20 and 5.00-rc4
for Aquantia NIC 1d6a:87b1
Product: Networking
Version: 2.5
Kernel Version: 5.00-rc4
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: IPV4
Assignee: stephen@networkplumber.org
Reporter: nicholas.johnson@outlook.com.au
Regression: No
Hardware Details:
==============================================================================
VEN:DEV = 1d6a:87b1 "Aquantia Corp. AQC107 NBase-T/IEEE 802.3bz Ethernet
Controller [AQtion]" Specifically, this is the newer AQC107S with "S"
meaning "Secure" and the company could never elaborate on how it is more
secure than the plain AQC107 (different VEN:DEV). This chip is inside
the Promise SANLink3 N1 Thunderbolt 3 10GbE adapter.
Severity:
==============================================================================
Marked high severity because it directly impacts user experience (cannot
get the network to come up on the adapter).
Back Story and Problem:
==============================================================================
In 5.00-rc4, on my Ubuntu desktop I have had extreme problems getting
the connection up (borderline impossible). If I did manage (after
messing with systemd *a lot*) then I got a dodgy connection with a lot
of DNS resolution problems. At first I blamed it on my desktop which had
a habit of corrupting the SSD partition tables on a regular basis, and
hence I assumed that systemd was corrupted or something.
Recently, I tried on my laptop with Arch Linux and 5.00-rc4 and it had
the exact same problem. It does not matter if you manually assign the
IPv4 address, or use DHCPv4.
Just now, on my laptop, I booted into 4.20.3-arch1-1-ARCH kernel from
Arch Linux and it worked straight away.
Expected behaviour:
==============================================================================
NetworkManager (or any tool) should bring the link up successfully.
Reproduction:
==============================================================================
Should be reproducible with a 5.00-rc4 kernel and the 1d6a:87b1 NIC. I
have no way of testing if other Aquantia devices are affected - I do not
own any others. I am using Ubuntu and Arch (both the very latest) and I
just cannot get NetworkManager to bring up the connection with 5.00-rc4
(happens effortlessly with 4.20 kernel). If you want NetworkManager to
manage the adapter, you may need to use the
10-globally-managed-devices.conf fix, depending on the version.
Technical Notes:
==============================================================================
It is hard to tell, but my gut tells me that it is IPv4-related. That
would explain why some sites could work. I have a native IPv6 service
from my ISP. The "ifconfig" command shows IPv6 addresses assigned, but
it is very difficult to get a DHCPv4-assigned address on the adapter.
A couple of times I did manage to get the adapter up in 5.00-rc4 with a
lot of messing around, and the resulting internet connection was
unreliable and unstable. DNS resolution seemed to be a big part of the
problem.
All BARs are assigned correctly. Other than that, dmesg does not give
anything interesting that I can tell, so if you want some debug logs
from somewhere then please tell me what you want and how I should do it.
There are a fair few lines of diff between 4.20 and 5.00-rc4 so there is
likely an oopsie in there.
Always Remembering My Manners:
==============================================================================
Thank you for reading my bug report!
Why Are Web Developers So Annoying?
==============================================================================
The preview removes all my newlines and nice formatting which I did so have fun
trying to read this big blob of unformatted text.
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply
* Re: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for coalesced packets
From: Stephen Hemminger @ 2019-02-23 16:46 UTC (permalink / raw)
To: Haiyang Zhang
Cc: haiyangz, sashal, linux-hyperv, kys, sthemmin, olaf, vkuznets,
davem, netdev, linux-kernel
In-Reply-To: <20190222182503.12160-1-haiyangz@linuxonhyperv.com>
On Fri, 22 Feb 2019 18:25:03 +0000
Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> Incoming packets may have IP header checksum verified by the host.
> They may not have IP header checksum computed after coalescing.
> This patch re-compute the checksum when necessary, otherwise the
> packets may be dropped, because Linux network stack always checks it.
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/net/hyperv/netvsc_drv.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 256adbd044f5..cf4897043e83 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -744,6 +744,14 @@ void netvsc_linkstatus_callback(struct net_device *net,
> schedule_delayed_work(&ndev_ctx->dwork, 0);
> }
>
> +static void netvsc_comp_ipcsum(struct sk_buff *skb)
> +{
> + struct iphdr *iph = (struct iphdr *)skb->data;
Can you use iphdr(skb) here?
> +
> + iph->check = 0;
> + iph->check = ip_fast_csum(iph, iph->ihl);
> +}
> +
> static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> struct netvsc_channel *nvchan)
> {
> @@ -770,9 +778,17 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
> /* skb is already created with CHECKSUM_NONE */
> skb_checksum_none_assert(skb);
>
> - /*
> - * In Linux, the IP checksum is always checked.
> - * Do L4 checksum offload if enabled and present.
> + /* Incoming packets may have IP header checksum verified by the host.
> + * They may not have IP header checksum computed after coalescing.
> + * We compute it here if the flags are set, because on Linux, the IP
> + * checksum is always checked.
> + */
> + if (csum_info && csum_info->receive.ip_checksum_value_invalid &&
> + csum_info->receive.ip_checksum_succeeded &&
> + skb->protocol == htons(ETH_P_IP))
> + netvsc_comp_ipcsum(skb);
Does this still handle for coalesced and non-coalesced packets
which are received with bad IP checksum? My concern is that you are
potentially correcting the checksum for a packet whose received checksum was bad.
> + /* Do L4 checksum offload if enabled and present.
> */
> if (csum_info && (net->features & NETIF_F_RXCSUM)) {
> if (csum_info->receive.tcp_checksum_succeeded ||
^ permalink raw reply
* [PATCH net-next 0/2] mv88e6xxx: Avoid false positive Lockdep splats
From: Andrew Lunn @ 2019-02-23 16:43 UTC (permalink / raw)
To: David Miller; +Cc: Vivien Didelot, Russell King, netdev, Andrew Lunn
When acquiring the GPIO interrupt line for the switch, it is possible
to trigger lockdep splats. These are false positives, the mutex is in
a different IRQ descriptor. But fix it anyway, since it could mask
real locking issues.
Andrew Lunn (2):
net: dsa: mv88e6xxx: Add lockdep classes to fix false positive splat
net: dsa: mv88e6xxx: Release lock while requesting IRQ
drivers/net/dsa/mv88e6xxx/chip.c | 10 ++++++++++
1 file changed, 10 insertions(+)
--
2.20.1
^ permalink raw reply
* [PATCH net-next 1/2] net: dsa: mv88e6xxx: Add lockdep classes to fix false positive splat
From: Andrew Lunn @ 2019-02-23 16:43 UTC (permalink / raw)
To: David Miller; +Cc: Vivien Didelot, Russell King, netdev, Andrew Lunn
In-Reply-To: <20190223164357.17284-1-andrew@lunn.ch>
The following false positive lockdep splat has been observed.
======================================================
WARNING: possible circular locking dependency detected
4.20.0+ #302 Not tainted
------------------------------------------------------
systemd-udevd/160 is trying to acquire lock:
edea6080 (&chip->reg_lock){+.+.}, at: __setup_irq+0x640/0x704
but task is already holding lock:
edff0340 (&desc->request_mutex){+.+.}, at: __setup_irq+0xa0/0x704
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&desc->request_mutex){+.+.}:
mutex_lock_nested+0x1c/0x24
__setup_irq+0xa0/0x704
request_threaded_irq+0xd0/0x150
mv88e6xxx_probe+0x41c/0x694 [mv88e6xxx]
mdio_probe+0x2c/0x54
really_probe+0x200/0x2c4
driver_probe_device+0x5c/0x174
__driver_attach+0xd8/0xdc
bus_for_each_dev+0x58/0x7c
bus_add_driver+0xe4/0x1f0
driver_register+0x7c/0x110
mdio_driver_register+0x24/0x58
do_one_initcall+0x74/0x2e8
do_init_module+0x60/0x1d0
load_module+0x1968/0x1ff4
sys_finit_module+0x8c/0x98
ret_fast_syscall+0x0/0x28
0xbedf2ae8
-> #0 (&chip->reg_lock){+.+.}:
__mutex_lock+0x50/0x8b8
mutex_lock_nested+0x1c/0x24
__setup_irq+0x640/0x704
request_threaded_irq+0xd0/0x150
mv88e6xxx_g2_irq_setup+0xcc/0x1b4 [mv88e6xxx]
mv88e6xxx_probe+0x44c/0x694 [mv88e6xxx]
mdio_probe+0x2c/0x54
really_probe+0x200/0x2c4
driver_probe_device+0x5c/0x174
__driver_attach+0xd8/0xdc
bus_for_each_dev+0x58/0x7c
bus_add_driver+0xe4/0x1f0
driver_register+0x7c/0x110
mdio_driver_register+0x24/0x58
do_one_initcall+0x74/0x2e8
do_init_module+0x60/0x1d0
load_module+0x1968/0x1ff4
sys_finit_module+0x8c/0x98
ret_fast_syscall+0x0/0x28
0xbedf2ae8
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&desc->request_mutex);
lock(&chip->reg_lock);
lock(&desc->request_mutex);
lock(&chip->reg_lock);
&desc->request_mutex refer to two different mutex. #1 is the GPIO for
the chip interrupt. #2 is the chained interrupt between global 1 and
global 2.
Add lockdep classes to the GPIO interrupt to avoid this.
Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index cc7ce06b6d58..7ee8607bf3a6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -442,12 +442,20 @@ static int mv88e6xxx_g1_irq_setup_common(struct mv88e6xxx_chip *chip)
static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
{
+ static struct lock_class_key lock_key;
+ static struct lock_class_key request_key;
int err;
err = mv88e6xxx_g1_irq_setup_common(chip);
if (err)
return err;
+ /* These lock classes tells lockdep that global 1 irqs are in
+ * a different category than their parent GPIO, so it won't
+ * report false recursion.
+ */
+ irq_set_lockdep_class(chip->irq, &lock_key, &request_key);
+
err = request_threaded_irq(chip->irq, NULL,
mv88e6xxx_g1_irq_thread_fn,
IRQF_ONESHOT | IRQF_SHARED,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 2/2] net: dsa: mv88e6xxx: Release lock while requesting IRQ
From: Andrew Lunn @ 2019-02-23 16:43 UTC (permalink / raw)
To: David Miller; +Cc: Vivien Didelot, Russell King, netdev, Andrew Lunn
In-Reply-To: <20190223164357.17284-1-andrew@lunn.ch>
There is no need to hold the register lock while requesting the GPIO
interrupt. By not holding it we can also avoid a false positive
lockdep splat.
Reported-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/mv88e6xxx/chip.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 7ee8607bf3a6..9cb17d3ef590 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -456,10 +456,12 @@ static int mv88e6xxx_g1_irq_setup(struct mv88e6xxx_chip *chip)
*/
irq_set_lockdep_class(chip->irq, &lock_key, &request_key);
+ mutex_unlock(&chip->reg_lock);
err = request_threaded_irq(chip->irq, NULL,
mv88e6xxx_g1_irq_thread_fn,
IRQF_ONESHOT | IRQF_SHARED,
dev_name(chip->dev), chip);
+ mutex_lock(&chip->reg_lock);
if (err)
mv88e6xxx_g1_irq_free_common(chip);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next v2 7/7] net: phy: marvell10g: add support for the 88x2110 PHY
From: Andrew Lunn @ 2019-02-23 15:41 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-8-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:44AM +0100, Maxime Chevallier wrote:
> This patch adds support for the 88x2110 PHY, which is similar to the
> already supported 88x3310 PHY without the SFP interface.
>
> It supports 10/100/1000BASET along with 2.5GBASET, 5GBASET and 10GBASET,
> with the same interface modes that are used by the 3310.
>
> This PHY don't have the same issue as the 88x3310 regarding 2.5/5G
> abilities, and correctly follows the 802.3bz standard to list the
> supported abilities.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Antoine Tenart <antoine.tenart@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 5/7] net: phy: marvell10g: Force reading of 2.5/5G
From: Andrew Lunn @ 2019-02-23 15:40 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-6-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:42AM +0100, Maxime Chevallier wrote:
> As per 802.3bz, if bit 14 of (1.11) "PMA Extended Abilities" indicates
> whether or not we should read register (1.21) "2.52/5G PMA Extended
> Abilities", which contains information on the support of 2.5GBASET and
> 5GBASET.
>
> After testing on several variants of PHYS of this family, it appears
> that bit 14 in (1.11) isn't always set when it should be.
>
> PHYs 88X3310 (on MacchiatoBin) and 88E2010 do support 2.5G and 5GBASET,
> but don't have 1.11.14 set. Their register 1.21 is filled with the
> correct values, indicating 2.5G and 5G support.
>
> PHYs 88E2110 do have their 1.11.14 bit set, as it should.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 4/7] net: phy: marvell10g: Use a #define for 88X3310 family id
From: Andrew Lunn @ 2019-02-23 15:39 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-5-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:41AM +0100, Maxime Chevallier wrote:
> The PHY ID corresponding to the 88X3310 is also used for other PHYs in
> the same family, such as the 88E2010. Use a #define for the PHY id, that
> ignores the last nibble.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 3/7] net: phy: marvell10g: Use 2500BASEX when using 2.5GBASET
From: Andrew Lunn @ 2019-02-23 15:38 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-4-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:40AM +0100, Maxime Chevallier wrote:
> The Marvell Alaska family of PHYs supports 2.5GBaseT and 5GBaseT modes,
> as defined in the 802.3bz specification.
>
> Upon establishing a 2.5GBASET link, the PHY will reconfigure it's MII
> interface to 2500BASEX.
>
> At 5G, the PHY will reconfigure it's interface to 5GBASE-R, but this
> mode isn't supported by any MAC for now.
>
> This was tested with :
> - The 88X3310, which is on the MacchiatoBin
> - The 88E2010, an Alaska PHY that has no fiber interfaces, and is
> limited to 5G maximum speed.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 2/7] net: phy: marvell10g: Use linkmode_set_bit helper instead of __set_bit
From: Andrew Lunn @ 2019-02-23 15:37 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-3-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:39AM +0100, Maxime Chevallier wrote:
> Cosmetic patch making use of helpers dedicated to linkmodes handling.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next v2 1/7] net: phy: marvell10g: Use get_features to get the PHY abilities
From: Andrew Lunn @ 2019-02-23 15:36 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit,
Russell King, linux-arm-kernel, Antoine Tenart, thomas.petazzoni,
gregory.clement, miquel.raynal, nadavh, stefanc, mw
In-Reply-To: <20190222233744.25735-2-maxime.chevallier@bootlin.com>
On Sat, Feb 23, 2019 at 12:37:38AM +0100, Maxime Chevallier wrote:
> The Alaska family of 10G PHYs has more abilities than the ones listed in
> PHY_10GBIT_FULL_FEATURES, the exact list depending on the model.
>
> Make use of the newly introduced .get_features call to build this list,
> using genphy_c45_pma_read_abilities to build the list of supported
> linkmodes, and adding autoneg ability based on what's reported by the AN
> MMD.
>
> .config_init is still used to validate the interface_mode.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write functions.
From: Andrew Lunn @ 2019-02-23 15:25 UTC (permalink / raw)
To: Parshuram Thombare
Cc: nicolas.ferre, davem, netdev, f.fainelli, hkallweit1,
linux-kernel, rafalc, piotrs, jank
In-Reply-To: <20190222201242.GA20889@lvlogina.cadence.com>
On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote:
> This patch modify MDIO read/write functions to support
> communication with C45 PHY in Cadence ethernet controller driver.
>
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---
> drivers/net/ethernet/cadence/macb.h | 15 +++++--
> drivers/net/ethernet/cadence/macb_main.c | 61 ++++++++++++++++++++++++-----
> 2 files changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index bed4ded..59c23e0 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -636,10 +636,17 @@
> #define GEM_CLK_DIV96 5
>
> /* Constants for MAN register */
> -#define MACB_MAN_SOF 1
> -#define MACB_MAN_WRITE 1
> -#define MACB_MAN_READ 2
> -#define MACB_MAN_CODE 2
> +#define MACB_MAN_C22_SOF 1
> +#define MACB_MAN_C22_WRITE 1
> +#define MACB_MAN_C22_READ 2
> +#define MACB_MAN_C22_CODE 2
> +
> +#define MACB_MAN_C45_SOF 0
> +#define MACB_MAN_C45_ADDR 0
> +#define MACB_MAN_C45_WRITE 1
> +#define MACB_MAN_C45_POST_READ_INCR 2
> +#define MACB_MAN_C45_READ 3
> +#define MACB_MAN_C45_CODE 2
>
> /* Capability mask bits */
> #define MACB_CAPS_ISR_CLEAR_ON_WRITE 0x00000001
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 4f4f8e5..2494abf 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -323,11 +323,30 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
> struct macb *bp = bus->priv;
> int value;
>
> - macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
> - | MACB_BF(RW, MACB_MAN_READ)
> - | MACB_BF(PHYA, mii_id)
> - | MACB_BF(REGA, regnum)
> - | MACB_BF(CODE, MACB_MAN_CODE)));
> + if (regnum & MII_ADDR_C45) {
> + macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_C45_SOF)
> + | MACB_BF(RW, MACB_MAN_C45_ADDR)
> + | MACB_BF(PHYA, mii_id)
> + | MACB_BF(REGA, (regnum >> 16) & 0x1F)
> + | MACB_BF(DATA, regnum & 0xFFFF)
> + | MACB_BF(CODE, MACB_MAN_C45_CODE)));
> +
> + /* wait for end of transfer */
> + while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> + cpu_relax();
You need a timeout here, and anywhere you wait for the hardware to
complete. Try to make use of readx_poll_timeout() variants.
Andrew
^ permalink raw reply
* Re: [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write functions.
From: Andrew Lunn @ 2019-02-23 15:23 UTC (permalink / raw)
To: Parshuram Raju Thombare
Cc: nicolas.ferre@microchip.com, davem@davemloft.net,
netdev@vger.kernel.org, f.fainelli@gmail.com,
hkallweit1@gmail.com, linux-kernel@vger.kernel.org,
Rafal Ciepiela, Piotr Sroka, Jan Kotas
In-Reply-To: <CO2PR07MB2469D90FD13642D668F8D281C1780@CO2PR07MB2469.namprd07.prod.outlook.com>
> >On Fri, Feb 22, 2019 at 08:12:42PM +0000, Parshuram Thombare wrote:
> >> This patch modify MDIO read/write functions to support communication
> >> with C45 PHY in Cadence ethernet controller driver.
> >
> >Hi Parshuram
> >
> >Are all versions of the MDIO controller capable of doing C45?
> >
> > Andrew
> Now driver support c22 and c45 PHY.
> Are you suggesting to add check for C45 PHY using is_c45 in phydev ?
You are unconditionally supporting C45. Are there versions of the
hardware which don't actually support C45? You have this endless loop:
+ /* wait for end of transfer */
+ while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+ cpu_relax();
If there is hardware which does not support C45, will this loop
forever?
Andrew
^ permalink raw reply
* Re: [PATCH] net: phy: realtek: Dummy IRQ calls for RTL8366RB
From: Andrew Lunn @ 2019-02-23 15:17 UTC (permalink / raw)
To: Linus Walleij; +Cc: Florian Fainelli, netdev, Heiner Kallweit
In-Reply-To: <20190223023639.27038-1-linus.walleij@linaro.org>
On Sat, Feb 23, 2019 at 03:36:39AM +0100, Linus Walleij wrote:
> This fixes a regression introduced by
> commit 0d2e778e38e0ddffab4bb2b0e9ed2ad5165c4bf7
> "net: phy: replace PHY_HAS_INTERRUPT with a check for
> config_intr and ack_interrupt".
>
> This assumes that a PHY cannot trigger interrupt unless
> it has .config_intr() or .ack_interrupt() implemented.
> A later patch makes the code assume both need to be
> implemented for interrupts to be present.
>
> But this PHY (which is inside a DSA) will happily
> fire interrupts without either callback.
Hi Linus
Can you disable these interrupts?
If you have dummy implementations, what is clearing the interrupt?
Andrew
^ permalink raw reply
* [PATCH net-next v3] route: Add multipath_hash in flowi_common to make user-define hash
From: wenxu @ 2019-02-23 14:53 UTC (permalink / raw)
To: nikolay, davem; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
Current fib_multipath_hash_policy can make hash based on the L3 or
L4. But it only work on the outer IP. So a specific tunnel always
has the same hash value. But a specific tunnel may contain so many
inner connections.
This patch provide a generic multipath_hash in floi_common. It can
make a user-define hash which can mix with L3 or L4 hash.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c | 2 +-
include/net/flow.h | 2 ++
include/net/ip_tunnels.h | 3 ++-
net/ipv4/ip_gre.c | 2 +-
net/ipv4/ip_tunnel.c | 6 +++---
net/ipv4/route.c | 4 ++++
6 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
index ad5a9b9..536c23c 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
@@ -305,7 +305,7 @@ static int mlxsw_sp_span_dmac(struct neigh_table *tbl,
parms = mlxsw_sp_ipip_netdev_parms4(to_dev);
ip_tunnel_init_flow(&fl4, parms.iph.protocol, *daddrp, *saddrp,
- 0, 0, parms.link, tun->fwmark);
+ 0, 0, parms.link, tun->fwmark, 0);
rt = ip_route_output_key(tun->net, &fl4);
if (IS_ERR(rt))
diff --git a/include/net/flow.h b/include/net/flow.h
index 93f2c9a..4da503a 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -40,6 +40,7 @@ struct flowi_common {
__u32 flowic_secid;
kuid_t flowic_uid;
struct flowi_tunnel flowic_tun_key;
+ __be32 flowic_multipath_hash;
};
union flowi_uli {
@@ -78,6 +79,7 @@ struct flowi4 {
#define flowi4_secid __fl_common.flowic_secid
#define flowi4_tun_key __fl_common.flowic_tun_key
#define flowi4_uid __fl_common.flowic_uid
+#define flowi4_multipath_hash __fl_common.flowic_multipath_hash
/* (saddr,daddr) must be grouped, same order as in IP header */
__be32 saddr;
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index f069f64..af64560 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -241,7 +241,7 @@ static inline void ip_tunnel_init_flow(struct flowi4 *fl4,
int proto,
__be32 daddr, __be32 saddr,
__be32 key, __u8 tos, int oif,
- __u32 mark)
+ __u32 mark, __u32 tun_inner_hash)
{
memset(fl4, 0, sizeof(*fl4));
fl4->flowi4_oif = oif;
@@ -251,6 +251,7 @@ static inline void ip_tunnel_init_flow(struct flowi4 *fl4,
fl4->flowi4_proto = proto;
fl4->fl4_gre_key = key;
fl4->flowi4_mark = mark;
+ fl4->flowi4_multipath_hash = tun_inner_hash;
}
int ip_tunnel_init(struct net_device *dev);
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index ccee941..88ad95e 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -578,7 +578,7 @@ static int gre_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
key = &info->key;
ip_tunnel_init_flow(&fl4, IPPROTO_GRE, key->u.ipv4.dst, key->u.ipv4.src,
tunnel_id_to_key32(key->tun_id), key->tos, 0,
- skb->mark);
+ skb->mark, skb_get_hash(skb));
rt = ip_route_output_key(dev_net(dev), &fl4);
if (IS_ERR(rt))
return PTR_ERR(rt);
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 893f013..3a03114 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -310,7 +310,7 @@ static int ip_tunnel_bind_dev(struct net_device *dev)
ip_tunnel_init_flow(&fl4, iph->protocol, iph->daddr,
iph->saddr, tunnel->parms.o_key,
RT_TOS(iph->tos), tunnel->parms.link,
- tunnel->fwmark);
+ tunnel->fwmark, 0);
rt = ip_route_output_key(tunnel->net, &fl4);
if (!IS_ERR(rt)) {
@@ -584,7 +584,7 @@ void ip_md_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
}
ip_tunnel_init_flow(&fl4, proto, key->u.ipv4.dst, key->u.ipv4.src,
tunnel_id_to_key32(key->tun_id), RT_TOS(tos),
- 0, skb->mark);
+ 0, skb->mark, skb_get_hash(skb));
if (tunnel->encap.type != TUNNEL_ENCAP_NONE)
goto tx_error;
@@ -738,7 +738,7 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct net_device *dev,
ip_tunnel_init_flow(&fl4, protocol, dst, tnl_params->saddr,
tunnel->parms.o_key, RT_TOS(tos), tunnel->parms.link,
- tunnel->fwmark);
+ tunnel->fwmark, skb_get_hash(skb));
if (ip_tunnel_encap(skb, tunnel, &protocol, &fl4) < 0)
goto tx_error;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index ecc12a7..6b9a8b7 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1820,6 +1820,7 @@ static void ip_multipath_l3_keys(const struct sk_buff *skb,
int fib_multipath_hash(const struct net *net, const struct flowi4 *fl4,
const struct sk_buff *skb, struct flow_keys *flkeys)
{
+ u32 multipath_hash = fl4->flowi4_multipath_hash;
struct flow_keys hash_keys;
u32 mhash;
@@ -1869,6 +1870,9 @@ int fib_multipath_hash(const struct net *net, const struct flowi4 *fl4,
break;
}
mhash = flow_hash_from_keys(&hash_keys);
+
+ if (multipath_hash)
+ mhash = jhash_2words(mhash, multipath_hash, 0);
return mhash >> 1;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v4] ip_tunnel: Add dst_cache support in lwtunnel_state of ip tunnel
From: wenxu @ 2019-02-23 13:32 UTC (permalink / raw)
To: davem; +Cc: netdev
From: wenxu <wenxu@ucloud.cn>
The lwtunnel_state is not init the dst_cache Which make the
ip_md_tunnel_xmit can't use the dst_cache. It will lookup
route table every packets.
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
net/core/lwtunnel.c | 16 ++++++++--------
net/ipv4/ip_tunnel_core.c | 18 ++++++++++++++++++
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
index 0b17175..4109095 100644
--- a/net/core/lwtunnel.c
+++ b/net/core/lwtunnel.c
@@ -122,18 +122,18 @@ int lwtunnel_build_state(u16 encap_type,
ret = -EOPNOTSUPP;
rcu_read_lock();
ops = rcu_dereference(lwtun_encaps[encap_type]);
- if (likely(ops && ops->build_state && try_module_get(ops->owner))) {
+ if (likely(ops && ops->build_state && try_module_get(ops->owner)))
found = true;
+ rcu_read_unlock();
+
+ if (found) {
ret = ops->build_state(encap, family, cfg, lws, extack);
if (ret)
module_put(ops->owner);
- }
- rcu_read_unlock();
-
- /* don't rely on -EOPNOTSUPP to detect match as build_state
- * handlers could return it
- */
- if (!found) {
+ } else {
+ /* don't rely on -EOPNOTSUPP to detect match as build_state
+ * handlers could return it
+ */
NL_SET_ERR_MSG_ATTR(extack, encap,
"LWT encapsulation type not supported");
}
diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
index 9a0e67b..c3f3d28 100644
--- a/net/ipv4/ip_tunnel_core.c
+++ b/net/ipv4/ip_tunnel_core.c
@@ -252,6 +252,14 @@ static int ip_tun_build_state(struct nlattr *attr,
tun_info = lwt_tun_info(new_state);
+#ifdef CONFIG_DST_CACHE
+ err = dst_cache_init(&tun_info->dst_cache, GFP_KERNEL);
+ if (err) {
+ lwtstate_free(new_state);
+ return err;
+ }
+#endif
+
if (tb[LWTUNNEL_IP_ID])
tun_info->key.tun_id = nla_get_be64(tb[LWTUNNEL_IP_ID]);
@@ -278,6 +286,15 @@ static int ip_tun_build_state(struct nlattr *attr,
return 0;
}
+static void ip_tun_destroy_state(struct lwtunnel_state *lwtstate)
+{
+#ifdef CONFIG_DST_CACHE
+ struct ip_tunnel_info *tun_info = lwt_tun_info(lwtstate);
+
+ dst_cache_destroy(&tun_info->dst_cache);
+#endif
+}
+
static int ip_tun_fill_encap_info(struct sk_buff *skb,
struct lwtunnel_state *lwtstate)
{
@@ -313,6 +330,7 @@ static int ip_tun_cmp_encap(struct lwtunnel_state *a, struct lwtunnel_state *b)
static const struct lwtunnel_encap_ops ip_tun_lwt_ops = {
.build_state = ip_tun_build_state,
+ .destroy_state = ip_tun_destroy_state,
.fill_encap = ip_tun_fill_encap_info,
.get_encap_size = ip_tun_encap_nlsize,
.cmp_encap = ip_tun_cmp_encap,
--
1.8.3.1
^ permalink raw reply related
* Re: [Outreachy kernel] [PATCH] net: ax25: Fix multiple blank lines warning
From: Julia Lawall @ 2019-02-23 13:22 UTC (permalink / raw)
To: Jules Irenge
Cc: jreuter, linux-hams, linux-kernel, davem, ralf, netdev,
outreachy-kernel
In-Reply-To: <20190223111751.14620-1-jbi.octave@gmail.com>
On Sat, 23 Feb 2019, Jules Irenge wrote:
You need a commit message here.
> Signed-off-by: Jules Irenge <jbi.octave@gmail.com>
> ---
> net/ax25/af_ax25.c | 2 --
This is the same file as in your previous patch, so they should be in a
series.
But please work on files in drivers/staging instead.
julia
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index 5048790a75f6..2da349b7854a 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -49,8 +49,6 @@
> #include <net/ip.h>
> #include <net/arp.h>
>
> -
> -
> HLIST_HEAD(ax25_list);
> DEFINE_SPINLOCK(ax25_list_lock);
>
> --
> 2.20.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190223111751.14620-1-jbi.octave%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ 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