* Re: [PATCH v2 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Song Liu @ 2018-09-25 18:54 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Kernel Team,
Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20180925152114.13537-4-guro@fb.com>
> On Sep 25, 2018, at 8:21 AM, Roman Gushchin <guro@fb.com> wrote:
>
> This commit introduced per-cpu cgroup local storage.
>
> Per-cpu cgroup local storage is very similar to simple cgroup storage
> (let's call it shared), except all the data is per-cpu.
>
> The main goal of per-cpu variant is to implement super fast
> counters (e.g. packet counters), which don't require neither
> lookups, neither atomic operations.
>
> From userspace's point of view, accessing a per-cpu cgroup storage
> is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> arrays).
>
> Writing to a per-cpu cgroup storage is not atomic, but is performed
> by copying longs, so some minimal atomicity is here, exactly
> as with other per-cpu maps.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf-cgroup.h | 20 ++++-
> include/linux/bpf.h | 1 +
> include/linux/bpf_types.h | 1 +
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/helpers.c | 8 +-
> kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> kernel/bpf/syscall.c | 11 ++-
> kernel/bpf/verifier.c | 15 +++-
> 8 files changed, 177 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 7e0c9a1d48b7..9bd907657f9b 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> };
>
> struct bpf_cgroup_storage {
> - struct bpf_storage_buffer *buf;
> + union {
> + struct bpf_storage_buffer *buf;
> + char __percpu *percpu_buf;
"char *" here looks weird. Did you mean to use "void *"?
> + };
> struct bpf_cgroup_storage_map *map;
> struct bpf_cgroup_storage_key key;
> struct list_head list;
> @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> struct bpf_map *map)
> {
> + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> + return BPF_CGROUP_STORAGE_PERCPU;
> +
> return BPF_CGROUP_STORAGE_SHARED;
> }
>
> @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> + void *value, u64 flags);
> +
> /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> ({ \
> @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> static inline void bpf_cgroup_storage_free(
> struct bpf_cgroup_storage *storage) {}
> +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> + void *value) {
> + return 0;
> +}
> +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> + void *key, void *value, u64 flags) {
> + return 0;
> +}
>
> #define cgroup_bpf_enabled (0)
> #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b457fbe7b70b..018299a595c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -274,6 +274,7 @@ struct bpf_prog_offload {
>
> enum bpf_cgroup_storage_type {
> BPF_CGROUP_STORAGE_SHARED,
> + BPF_CGROUP_STORAGE_PERCPU,
> __BPF_CGROUP_STORAGE_MAX
> };
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index c9bd6fb765b0..5432f4c9f50e 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> #endif
> #ifdef CONFIG_CGROUP_BPF
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> #endif
> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..e2070d819e04 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -127,6 +127,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_SOCKHASH,
> BPF_MAP_TYPE_CGROUP_STORAGE,
> BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> };
>
> enum bpf_prog_type {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index e42f8789b7ea..1f21ef1c4ad3 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> */
> enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> struct bpf_cgroup_storage *storage;
> + void *ptr = NULL;
Not necessary to initialize to NULL.
>
> storage = this_cpu_read(bpf_cgroup_storage[stype]);
>
> - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + ptr = &READ_ONCE(storage->buf)->data[0];
> + else
> + ptr = this_cpu_ptr(storage->percpu_buf);
> +
> + return (unsigned long)ptr;
> }
>
> const struct bpf_func_proto bpf_get_local_storage_proto = {
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 6742292fb39e..d991355b5b46 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> return 0;
> }
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> + void *value)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* per_cpu areas are zero-filled and bpf programs can only
> + * access 'value_size' of them, so copying rounded areas
> + * will not leak any kernel data
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(value + off,
> + per_cpu_ptr(storage->percpu_buf, cpu), size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + if (unlikely(map_flags & BPF_EXIST))
> + return -EINVAL;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* the user space will provide round_up(value_size, 8) bytes that
> + * will be copied into per-cpu area. bpf programs can only access
> + * value_size of it. During lookup the same extra bytes will be
> + * returned or zeros which were zero-filled by percpu_alloc,
> + * so no kernel data leaks possible
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> + value + off, size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> static int cgroup_storage_get_next_key(struct bpf_map *_map, void *_key,
> void *_next_key)
> {
> @@ -292,55 +357,98 @@ struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(struct bpf_prog *prog,
> {
> struct bpf_cgroup_storage *storage;
> struct bpf_map *map;
> + gfp_t flags;
> + size_t size;
> u32 pages;
>
> map = prog->aux->cgroup_storage[stype];
> if (!map)
> return NULL;
>
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + size = sizeof(struct bpf_storage_buffer) + map->value_size;
> + pages = round_up(sizeof(struct bpf_cgroup_storage) + size,
> + PAGE_SIZE) >> PAGE_SHIFT;
> + } else {
> + size = map->value_size;
> + pages = round_up(round_up(size, 8) * num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> + }
> +
> if (bpf_map_charge_memlock(map, pages))
> return ERR_PTR(-EPERM);
>
> storage = kmalloc_node(sizeof(struct bpf_cgroup_storage),
> __GFP_ZERO | GFP_USER, map->numa_node);
> - if (!storage) {
> - bpf_map_uncharge_memlock(map, pages);
> - return ERR_PTR(-ENOMEM);
> - }
> + if (!storage)
> + goto enomem;
>
> - storage->buf = kmalloc_node(sizeof(struct bpf_storage_buffer) +
> - map->value_size, __GFP_ZERO | GFP_USER,
> - map->numa_node);
> - if (!storage->buf) {
> - bpf_map_uncharge_memlock(map, pages);
> - kfree(storage);
> - return ERR_PTR(-ENOMEM);
> + flags = __GFP_ZERO | GFP_USER;
> +
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + storage->buf = kmalloc_node(size, flags, map->numa_node);
> + if (!storage->buf)
> + goto enomem;
> + } else {
> + storage->percpu_buf = __alloc_percpu_gfp(size, 8, flags);
> + if (!storage->percpu_buf)
> + goto enomem;
> }
>
> storage->map = (struct bpf_cgroup_storage_map *)map;
>
> return storage;
> +
> +enomem:
> + bpf_map_uncharge_memlock(map, pages);
> + kfree(storage);
> + return ERR_PTR(-ENOMEM);
> +}
> +
> +static void free_cgroup_storage_rcu(struct rcu_head *rcu)
Maybe rename as free_shared_cgroup_storage_rcu()?
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + kfree(storage->buf);
> + kfree(storage);
> +}
> +
> +static void free_percpu_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + free_percpu(storage->percpu_buf);
> + kfree(storage);
> }
>
> void bpf_cgroup_storage_free(struct bpf_cgroup_storage *storage)
> {
> - u32 pages;
> + enum bpf_cgroup_storage_type stype;
> struct bpf_map *map;
> + u32 pages;
>
> if (!storage)
> return;
>
> map = &storage->map->map;
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + stype = cgroup_storage_type(map);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + pages = round_up(sizeof(struct bpf_cgroup_storage) +
> + sizeof(struct bpf_storage_buffer) +
> + map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + else
> + pages = round_up(round_up(map->value_size, 8) *
> + num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> +
> bpf_map_uncharge_memlock(map, pages);
>
> - kfree_rcu(storage->buf, rcu);
> - kfree_rcu(storage, rcu);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + call_rcu(&storage->rcu, free_cgroup_storage_rcu);
> + else
> + call_rcu(&storage->rcu, free_percpu_cgroup_storage_rcu);
> }
>
> void bpf_cgroup_storage_link(struct bpf_cgroup_storage *storage,
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8c91d2b41b1e..5742df21598c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -686,7 +686,8 @@ static int map_lookup_elem(union bpf_attr *attr)
>
> if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
> - map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> + map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> value_size = round_up(map->value_size, 8) * num_possible_cpus();
> else if (IS_FD_MAP(map))
> value_size = sizeof(u32);
> @@ -705,6 +706,8 @@ static int map_lookup_elem(union bpf_attr *attr)
> err = bpf_percpu_hash_copy(map, key, value);
> } else if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
> err = bpf_percpu_array_copy(map, key, value);
> + } else if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
> + err = bpf_percpu_cgroup_storage_copy(map, key, value);
> } else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE) {
> err = bpf_stackmap_copy(map, key, value);
> } else if (IS_FD_ARRAY(map)) {
> @@ -774,7 +777,8 @@ static int map_update_elem(union bpf_attr *attr)
>
> if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
> - map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> + map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> value_size = round_up(map->value_size, 8) * num_possible_cpus();
> else
> value_size = map->value_size;
> @@ -809,6 +813,9 @@ static int map_update_elem(union bpf_attr *attr)
> err = bpf_percpu_hash_update(map, key, value, attr->flags);
> } else if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
> err = bpf_percpu_array_update(map, key, value, attr->flags);
> + } else if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
> + err = bpf_percpu_cgroup_storage_update(map, key, value,
> + attr->flags);
> } else if (IS_FD_ARRAY(map)) {
> rcu_read_lock();
> err = bpf_fd_array_map_update_elem(map, f.file, key, value,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e75f36de91d6..d94073deb68a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2074,6 +2074,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
> goto error;
> break;
> case BPF_MAP_TYPE_CGROUP_STORAGE:
> + case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
> if (func_id != BPF_FUNC_get_local_storage)
> goto error;
> break;
> @@ -2164,7 +2165,8 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
> goto error;
> break;
> case BPF_FUNC_get_local_storage:
> - if (map->map_type != BPF_MAP_TYPE_CGROUP_STORAGE)
> + if (map->map_type != BPF_MAP_TYPE_CGROUP_STORAGE &&
> + map->map_type != BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> goto error;
> break;
> case BPF_FUNC_sk_select_reuseport:
> @@ -5049,6 +5051,12 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> return 0;
> }
>
> +static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
> +{
> + return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
> +}
> +
> /* look for pseudo eBPF instructions that access map FDs and
> * replace them with actual map pointers
> */
> @@ -5139,10 +5147,9 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
> }
> env->used_maps[env->used_map_cnt++] = map;
>
> - if (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE &&
> + if (bpf_map_is_cgroup_storage(map) &&
> bpf_cgroup_storage_assign(env->prog, map)) {
> - verbose(env,
> - "only one cgroup storage is allowed\n");
> + verbose(env, "only one cgroup storage of each type is allowed\n");
> fdput(f);
> return -EBUSY;
> }
> --
> 2.17.1
>
^ permalink raw reply
* requesting stable backport of BPF security fix (commit dd066823db2ac4e22f721ec85190817b58059a54)
From: Jann Horn @ 2018-09-25 12:46 UTC (permalink / raw)
To: Network Development, David S. Miller
Cc: Alexei Starovoitov, Daniel Borkmann, security
Hi!
Per the policy at Documentation/networking/netdev-FAQ.rst, I'm sending
this to netdev@ and davem, rather than stable@; with a CC to security@
because I believe that this is a security process issue.
Upstream commit dd066823db2ac4e22f721ec85190817b58059a54
("bpf/verifier: disallow pointer subtraction") fixes a security bug
(kernel pointer leak to unprivileged userspace). The fix has been in
Linus' tree since about a week ago, but the patch still doesn't appear
in Greg's linux-4.18.y linux-stable-rc repo, in Greg's 4.18
stable-queue, or in davem's stable queue at
http://patchwork.ozlabs.org/bundle/davem/stable/?state=* .
Please queue it up for backporting.
I am curious: Why was this not queued up for stable immediately? Or
was it, and I looked in the wrong place?
^ permalink raw reply
* [REBASE PATCH net-next v9 4/4] net: vhost: add rx busy polling in tx path
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch improves the guest receive performance.
On the handle_tx side, we poll the sock receive queue at the
same time. handle_rx do that in the same way.
We set the poll-us=100us and use the netperf to test throughput
and mean latency. When running the tests, the vhost-net kthread
of that VM, is alway 100% CPU. The commands are shown as below.
Rx performance is greatly improved by this patch. There is not
notable performance change on tx with this series though. This
patch is useful for bi-directional traffic.
netperf -H IP -t TCP_STREAM -l 20 -- -O "THROUGHPUT, THROUGHPUT_UNITS, MEAN_LATENCY"
Topology:
[Host] ->linux bridge -> tap vhost-net ->[Guest]
TCP_STREAM:
* Without the patch: 19842.95 Mbps, 6.50 us mean latency
* With the patch: 37598.20 Mbps, 3.43 us mean latency
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/vhost/net.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index ac0b954..015abf3 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -547,34 +547,27 @@ static void vhost_net_busy_poll(struct vhost_net *net,
mutex_unlock(&vq->mutex);
}
-
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
- struct vhost_net_virtqueue *nvq,
+ struct vhost_net_virtqueue *tnvq,
unsigned int *out_num, unsigned int *in_num,
struct msghdr *msghdr, bool *busyloop_intr)
{
- struct vhost_virtqueue *vq = &nvq->vq;
- unsigned long uninitialized_var(endtime);
- int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+ struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
+ struct vhost_virtqueue *rvq = &rnvq->vq;
+ struct vhost_virtqueue *tvq = &tnvq->vq;
+
+ int r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
out_num, in_num, NULL, NULL);
- if (r == vq->num && vq->busyloop_timeout) {
+ if (r == tvq->num && tvq->busyloop_timeout) {
/* Flush batched packets first */
- if (!vhost_sock_zcopy(vq->private_data))
- vhost_tx_batch(net, nvq, vq->private_data, msghdr);
- preempt_disable();
- endtime = busy_clock() + vq->busyloop_timeout;
- while (vhost_can_busy_poll(endtime)) {
- if (vhost_has_work(vq->dev)) {
- *busyloop_intr = true;
- break;
- }
- if (!vhost_vq_avail_empty(vq->dev, vq))
- break;
- cpu_relax();
- }
- preempt_enable();
- r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+ if (!vhost_sock_zcopy(tvq->private_data))
+ // vhost_net_signal_used(tnvq);
+ vhost_tx_batch(net, tnvq, tvq->private_data, msghdr);
+
+ vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, false);
+
+ r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
out_num, in_num, NULL, NULL);
}
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Factor out generic busy polling logic and will be
used for in tx path in the next patch. And with the patch,
qemu can set differently the busyloop_timeout for rx queue.
To avoid duplicate codes, introduce the helper functions:
* sock_has_rx_data(changed from sk_has_rx_data)
* vhost_net_busy_poll_try_queue
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/vhost/net.c | 110 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 70 insertions(+), 40 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 5fe57ab..ac0b954 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -480,6 +480,74 @@ static void vhost_tx_batch(struct vhost_net *net,
nvq->batched_xdp = 0;
}
+static int sock_has_rx_data(struct socket *sock)
+{
+ if (unlikely(!sock))
+ return 0;
+
+ if (sock->ops->peek_len)
+ return sock->ops->peek_len(sock);
+
+ return skb_queue_empty(&sock->sk->sk_receive_queue);
+}
+
+static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
+{
+ if (!vhost_vq_avail_empty(&net->dev, vq)) {
+ vhost_poll_queue(&vq->poll);
+ } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ vhost_poll_queue(&vq->poll);
+ }
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool *busyloop_intr,
+ bool poll_rx)
+{
+ unsigned long busyloop_timeout;
+ unsigned long endtime;
+ struct socket *sock;
+ struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
+
+ mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+ vhost_disable_notify(&net->dev, vq);
+ sock = rvq->private_data;
+
+ busyloop_timeout = poll_rx ? rvq->busyloop_timeout:
+ tvq->busyloop_timeout;
+
+ preempt_disable();
+ endtime = busy_clock() + busyloop_timeout;
+
+ while (vhost_can_busy_poll(endtime)) {
+ if (vhost_has_work(&net->dev)) {
+ *busyloop_intr = true;
+ break;
+ }
+
+ if ((sock_has_rx_data(sock) &&
+ !vhost_vq_avail_empty(&net->dev, rvq)) ||
+ !vhost_vq_avail_empty(&net->dev, tvq))
+ break;
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (poll_rx || sock_has_rx_data(sock))
+ vhost_net_busy_poll_try_queue(net, vq);
+ else if (!poll_rx) /* On tx here, sock has no rx data. */
+ vhost_enable_notify(&net->dev, rvq);
+
+ mutex_unlock(&vq->mutex);
+}
+
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_net_virtqueue *nvq,
unsigned int *out_num, unsigned int *in_num,
@@ -897,16 +965,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
return len;
}
-static int sk_has_rx_data(struct sock *sk)
-{
- struct socket *sock = sk->sk_socket;
-
- if (sock->ops->peek_len)
- return sock->ops->peek_len(sock);
-
- return skb_queue_empty(&sk->sk_receive_queue);
-}
-
static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
bool *busyloop_intr)
{
@@ -914,41 +972,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *rvq = &rnvq->vq;
struct vhost_virtqueue *tvq = &tnvq->vq;
- unsigned long uninitialized_var(endtime);
int len = peek_head_len(rnvq, sk);
- if (!len && tvq->busyloop_timeout) {
+ if (!len && rvq->busyloop_timeout) {
/* Flush batched heads first */
vhost_net_signal_used(rnvq);
/* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
- vhost_disable_notify(&net->dev, tvq);
-
- preempt_disable();
- endtime = busy_clock() + tvq->busyloop_timeout;
-
- while (vhost_can_busy_poll(endtime)) {
- if (vhost_has_work(&net->dev)) {
- *busyloop_intr = true;
- break;
- }
- if ((sk_has_rx_data(sk) &&
- !vhost_vq_avail_empty(&net->dev, rvq)) ||
- !vhost_vq_avail_empty(&net->dev, tvq))
- break;
- cpu_relax();
- }
-
- preempt_enable();
-
- if (!vhost_vq_avail_empty(&net->dev, tvq)) {
- vhost_poll_queue(&tvq->poll);
- } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
- vhost_disable_notify(&net->dev, tvq);
- vhost_poll_queue(&tvq->poll);
- }
-
- mutex_unlock(&tvq->mutex);
+ vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
len = peek_head_len(rnvq, sk);
}
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 2/4] net: vhost: replace magic number of lock annotation
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Use the VHOST_NET_VQ_XXX as a subclass for mutex_lock_nested.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 1bff6bc..5fe57ab 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -856,7 +856,7 @@ static void handle_tx(struct vhost_net *net)
struct vhost_virtqueue *vq = &nvq->vq;
struct socket *sock;
- mutex_lock(&vq->mutex);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
sock = vq->private_data;
if (!sock)
goto out;
@@ -921,7 +921,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
/* Flush batched heads first */
vhost_net_signal_used(rnvq);
/* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&tvq->mutex, 1);
+ mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
vhost_disable_notify(&net->dev, tvq);
preempt_disable();
@@ -1063,7 +1063,7 @@ static void handle_rx(struct vhost_net *net)
__virtio16 num_buffers;
int recv_pkts = 0;
- mutex_lock_nested(&vq->mutex, 0);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
sock = vq->private_data;
if (!sock)
goto out;
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 1/4] net: vhost: lock the vqs one by one
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch changes the way that lock all vqs
at the same, to lock them one by one. It will
be used for next patch to avoid the deadlock.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b13c6b4..f52008b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
{
int i;
- for (i = 0; i < d->nvqs; ++i)
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i]->mutex);
__vhost_vq_meta_reset(d->vqs[i]);
+ mutex_unlock(&d->vqs[i]->mutex);
+ }
}
static void vhost_vq_reset(struct vhost_dev *dev,
@@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
#define vhost_get_used(vq, x, ptr) \
vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
-static void vhost_dev_lock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_lock_nested(&d->vqs[i]->mutex, i);
-}
-
-static void vhost_dev_unlock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_unlock(&d->vqs[i]->mutex);
-}
-
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
if (msg->iova <= vq_msg->iova &&
msg->iova + msg->size - 1 >= vq_msg->iova &&
vq_msg->type == VHOST_IOTLB_MISS) {
+ mutex_lock(&node->vq->mutex);
vhost_poll_queue(&node->vq->poll);
+ mutex_unlock(&node->vq->mutex);
+
list_del(&node->node);
kfree(node);
}
@@ -986,7 +978,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
int ret = 0;
mutex_lock(&dev->mutex);
- vhost_dev_lock_vqs(dev);
switch (msg->type) {
case VHOST_IOTLB_UPDATE:
if (!dev->iotlb) {
@@ -1020,7 +1011,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
break;
}
- vhost_dev_unlock_vqs(dev);
mutex_unlock(&dev->mutex);
return ret;
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 0/4] net: vhost: improve performance when enable busyloop
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patches improve the guest receive performance.
On the handle_tx side, we poll the sock receive queue
at the same time. handle_rx do that in the same way.
For more performance report, see patch 4
Tonghao Zhang (4):
net: vhost: lock the vqs one by one
net: vhost: replace magic number of lock annotation
net: vhost: factor out busy polling logic to vhost_net_busy_poll()
net: vhost: add rx busy polling in tx path
drivers/vhost/net.c | 147 +++++++++++++++++++++++++++++---------------------
drivers/vhost/vhost.c | 24 +++------
2 files changed, 92 insertions(+), 79 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH net-next v6 17/23] zinc: Curve25519 generic C implementations and selftest
From: Joe Perches @ 2018-09-25 18:38 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, netdev, linux-crypto, davem,
gregkh
Cc: Samuel Neves, Andy Lutomirski, Jean-Philippe Aumasson,
Karthikeyan Bhargavan
In-Reply-To: <20180925145622.29959-18-Jason@zx2c4.com>
On Tue, 2018-09-25 at 16:56 +0200, Jason A. Donenfeld wrote:
> This contains two formally verified C implementations of the Curve25519
> scalar multiplication function, one for 32-bit systems, and one for
> 64-bit systems whose compiler supports efficient 128-bit integer types.
> Not only are these implementations formally verified, but they are also
> the fastest available C implementations. They have been modified to be
> friendly to kernel space and to be generally less horrendous looking,
> but still an effort has been made to retain their formally verified
> characteristic, and so the C might look slightly unidiomatic.
[]
> diff --git a/lib/zinc/curve25519/curve25519-fiat32.h b/lib/zinc/curve25519/curve25519-fiat32.h
[]
> +static __always_inline void fe_freeze(u32 out[10], const u32 in1[10])
> +{
> + { const u32 x17 = in1[9];
> + { const u32 x18 = in1[8];
> + { const u32 x16 = in1[7];
> + { const u32 x14 = in1[6];
> + { const u32 x12 = in1[5];
> + { const u32 x10 = in1[4];
> + { const u32 x8 = in1[3];
> + { const u32 x6 = in1[2];
> + { const u32 x4 = in1[1];
> + { const u32 x2 = in1[0];
> + { u32 x20; u8/*bool*/ x21 = subborrow_u26(0x0, x2, 0x3ffffed, &x20);
> + { u32 x23; u8/*bool*/ x24 = subborrow_u25(x21, x4, 0x1ffffff, &x23);
> + { u32 x26; u8/*bool*/ x27 = subborrow_u26(x24, x6, 0x3ffffff, &x26);
> + { u32 x29; u8/*bool*/ x30 = subborrow_u25(x27, x8, 0x1ffffff, &x29);
> + { u32 x32; u8/*bool*/ x33 = subborrow_u26(x30, x10, 0x3ffffff, &x32);
> + { u32 x35; u8/*bool*/ x36 = subborrow_u25(x33, x12, 0x1ffffff, &x35);
> + { u32 x38; u8/*bool*/ x39 = subborrow_u26(x36, x14, 0x3ffffff, &x38);
> + { u32 x41; u8/*bool*/ x42 = subborrow_u25(x39, x16, 0x1ffffff, &x41);
> + { u32 x44; u8/*bool*/ x45 = subborrow_u26(x42, x18, 0x3ffffff, &x44);
> + { u32 x47; u8/*bool*/ x48 = subborrow_u25(x45, x17, 0x1ffffff, &x47);
> + { u32 x49 = cmovznz32(x48, 0x0, 0xffffffff);
> + { u32 x50 = (x49 & 0x3ffffed);
> + { u32 x52; u8/*bool*/ x53 = addcarryx_u26(0x0, x20, x50, &x52);
> + { u32 x54 = (x49 & 0x1ffffff);
> + { u32 x56; u8/*bool*/ x57 = addcarryx_u25(x53, x23, x54, &x56);
> + { u32 x58 = (x49 & 0x3ffffff);
> + { u32 x60; u8/*bool*/ x61 = addcarryx_u26(x57, x26, x58, &x60);
> + { u32 x62 = (x49 & 0x1ffffff);
> + { u32 x64; u8/*bool*/ x65 = addcarryx_u25(x61, x29, x62, &x64);
> + { u32 x66 = (x49 & 0x3ffffff);
> + { u32 x68; u8/*bool*/ x69 = addcarryx_u26(x65, x32, x66, &x68);
> + { u32 x70 = (x49 & 0x1ffffff);
> + { u32 x72; u8/*bool*/ x73 = addcarryx_u25(x69, x35, x70, &x72);
> + { u32 x74 = (x49 & 0x3ffffff);
> + { u32 x76; u8/*bool*/ x77 = addcarryx_u26(x73, x38, x74, &x76);
> + { u32 x78 = (x49 & 0x1ffffff);
> + { u32 x80; u8/*bool*/ x81 = addcarryx_u25(x77, x41, x78, &x80);
> + { u32 x82 = (x49 & 0x3ffffff);
> + { u32 x84; u8/*bool*/ x85 = addcarryx_u26(x81, x44, x82, &x84);
> + { u32 x86 = (x49 & 0x1ffffff);
> + { u32 x88; addcarryx_u25(x85, x47, x86, &x88);
> + out[0] = x52;
> + out[1] = x56;
> + out[2] = x60;
> + out[3] = x64;
> + out[4] = x68;
> + out[5] = x72;
> + out[6] = x76;
> + out[7] = x80;
> + out[8] = x84;
> + out[9] = x88;
> + }}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}
> +}
Unidiomatic might be a stretch here...
[]
and elsewhere...
> +static void fe_add_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> +{
> + { const u32 x20 = in1[9];
> + { const u32 x21 = in1[8];
> + { const u32 x19 = in1[7];
> + { const u32 x17 = in1[6];
> + { const u32 x15 = in1[5];
> + { const u32 x13 = in1[4];
> + { const u32 x11 = in1[3];
> + { const u32 x9 = in1[2];
> + { const u32 x7 = in1[1];
> + { const u32 x5 = in1[0];
> + { const u32 x38 = in2[9];
> + { const u32 x39 = in2[8];
> + { const u32 x37 = in2[7];
> + { const u32 x35 = in2[6];
> + { const u32 x33 = in2[5];
> + { const u32 x31 = in2[4];
> + { const u32 x29 = in2[3];
> + { const u32 x27 = in2[2];
> + { const u32 x25 = in2[1];
> + { const u32 x23 = in2[0];
> + out[0] = (x5 + x23);
> + out[1] = (x7 + x25);
> + out[2] = (x9 + x27);
> + out[3] = (x11 + x29);
> + out[4] = (x13 + x31);
> + out[5] = (x15 + x33);
> + out[6] = (x17 + x35);
> + out[7] = (x19 + x37);
> + out[8] = (x21 + x39);
> + out[9] = (x20 + x38);
> + }}}}}}}}}}}}}}}}}}}}
> +}
etc...
^ permalink raw reply
* Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
From: Florian Fainelli @ 2018-09-25 18:35 UTC (permalink / raw)
To: Woojung.Huh, netdev
Cc: davem, UNGLinuxDriver, steve.glendinning, keescook, akurz,
hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <BN6PR1101MB2130D18F03C79BC1E54C9FBAE7160@BN6PR1101MB2130.namprd11.prod.outlook.com>
On 09/25/2018 10:32 AM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
>
>>>> + if (pdata->wol == 0)
>>>> + return -EINVAL;
>>>> +
>>> It will make function return when disabling WOL.
>>
>> Huh, yes, good point.
>>
>>> Is there other place handling this scenario?
>>
>> How do you mean?
>>
> I meant there is another path I might miss when disabling WOL
> than this xxx_set_wol().
I don't think so, at least not from the ethtool perspective, this should
fix the issue before, and simplifying the code, since all we are doing
it taking a bitmask, checking each bit we support, and again, make it
the same bitmask in pdata->wol, can you test that? If you have a new
enough version of ethtool, try using: ethtool -s <iface> wol f, which
was added recently and which this driver does not support:
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index a9991c5f4736..2e37028ef6ca 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device
*netdev,
if (ret < 0)
return ret;
- pdata->wol = 0;
- if (wol->wolopts & WAKE_UCAST)
- pdata->wol |= WAKE_UCAST;
- if (wol->wolopts & WAKE_MCAST)
- pdata->wol |= WAKE_MCAST;
- if (wol->wolopts & WAKE_BCAST)
- pdata->wol |= WAKE_BCAST;
- if (wol->wolopts & WAKE_MAGIC)
- pdata->wol |= WAKE_MAGIC;
- if (wol->wolopts & WAKE_PHY)
- pdata->wol |= WAKE_PHY;
- if (wol->wolopts & WAKE_ARP)
- pdata->wol |= WAKE_ARP;
+ if (pdata->wol & ~WAKE_ALL)
+ return -EINVAL;
+
+ pdata->wol = wol->wolopts;
device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
--
Florian
^ permalink raw reply related
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Joe Perches @ 2018-09-25 18:33 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, netdev, linux-crypto, davem,
gregkh
Cc: Samuel Neves, Andy Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <20180925145622.29959-3-Jason@zx2c4.com>
On Tue, 2018-09-25 at 16:56 +0200, Jason A. Donenfeld wrote:
> Zinc stands for "Zinc Is Neat Crypto" or "Zinc as IN Crypto" or maybe
> just "Zx2c4's INsane Cryptolib." It's also short, easy to type, and
> plays nicely with the recent trend of naming crypto libraries after
> elements. The guiding principle is "don't overdo it". It's less of a
> library and more of a directory tree for organizing well-curated direct
> implementations of cryptography primitives.
[]
> diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
> new file mode 100644
> index 000000000000..a61c80d676cb
> --- /dev/null
> +++ b/lib/zinc/Makefile
> @@ -0,0 +1,3 @@
> +ccflags-y := -O2
> +ccflags-y += -D'pr_fmt(fmt)="zinc: " fmt'
> +ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
I think the -Dpr_fmt is especially odd and not
really acceptable as it not used anywhere else
in the kernel.
^ permalink raw reply
* [PATCH net-next 2/2] net: phy: marvell: Avoid unnecessary soft reset
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
In-Reply-To: <20180925182846.30042-1-f.fainelli@gmail.com>
The BMCR.RESET bit on the Marvell PHYs has a special meaning in that
it commits the register writes into the HW for it to latch and be
configured appropriately. Doing software resets causes link drops, and
this is unnecessary disruption if nothing changed.
Determine from marvell_set_polarity()'s return code whether the register value
was changed and if it was, propagate that to the logic that hits the software
reset bit.
This avoids doing unnecessary soft reset if the PHY is configured in
the same state it was previously, this also eliminates the need for a
m88e1111_config_aneg() function since it now is the same as
marvell_config_aneg().
Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/marvell.c | 63 +++++++++++++--------------------------
1 file changed, 21 insertions(+), 42 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f7c69ca34056..b55a7376bfdc 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -265,7 +265,7 @@ static int marvell_set_polarity(struct phy_device *phydev, int polarity)
return err;
}
- return 0;
+ return val != reg;
}
static int marvell_set_downshift(struct phy_device *phydev, bool enable,
@@ -287,12 +287,15 @@ static int marvell_set_downshift(struct phy_device *phydev, bool enable,
static int marvell_config_aneg(struct phy_device *phydev)
{
+ int changed = 0;
int err;
err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
if (err < 0)
return err;
+ changed = err;
+
err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
MII_M1111_PHY_LED_DIRECT);
if (err < 0)
@@ -302,7 +305,7 @@ static int marvell_config_aneg(struct phy_device *phydev)
if (err < 0)
return err;
- if (phydev->autoneg != AUTONEG_ENABLE) {
+ if (phydev->autoneg != AUTONEG_ENABLE || changed) {
/* A write to speed/duplex bits (that is performed by
* genphy_config_aneg() call above) must be followed by
* a software reset. Otherwise, the write has no effect.
@@ -350,42 +353,6 @@ static int m88e1101_config_aneg(struct phy_device *phydev)
return marvell_config_aneg(phydev);
}
-static int m88e1111_config_aneg(struct phy_device *phydev)
-{
- int err;
-
- /* The Marvell PHY has an errata which requires
- * that certain registers get written in order
- * to restart autonegotiation
- */
- err = genphy_soft_reset(phydev);
-
- err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
- if (err < 0)
- return err;
-
- err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
- MII_M1111_PHY_LED_DIRECT);
- if (err < 0)
- return err;
-
- err = genphy_config_aneg(phydev);
- if (err < 0)
- return err;
-
- if (phydev->autoneg != AUTONEG_ENABLE) {
- /* A write to speed/duplex bits (that is performed by
- * genphy_config_aneg() call above) must be followed by
- * a software reset. Otherwise, the write has no effect.
- */
- err = genphy_soft_reset(phydev);
- if (err < 0)
- return err;
- }
-
- return 0;
-}
-
#ifdef CONFIG_OF_MDIO
/* Set and/or override some configuration registers based on the
* marvell,reg-init property stored in the of_node for the phydev.
@@ -479,6 +446,7 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
static int m88e1121_config_aneg(struct phy_device *phydev)
{
+ int changed = 0;
int err = 0;
if (phy_interface_is_rgmii(phydev)) {
@@ -487,15 +455,26 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
return err;
}
- err = genphy_soft_reset(phydev);
+ err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
if (err < 0)
return err;
- err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
+ changed = err;
+
+ err = genphy_config_aneg(phydev);
if (err < 0)
return err;
- return genphy_config_aneg(phydev);
+ if (phydev->autoneg != autoneg || changed) {
+ /* A software reset is used to ensure a "commit" of the
+ * changes is done.
+ */
+ err = genphy_soft_reset(phydev);
+ if (err < 0)
+ return err;
+ }
+
+ return 0;
}
static int m88e1318_config_aneg(struct phy_device *phydev)
@@ -2067,7 +2046,7 @@ static struct phy_driver marvell_drivers[] = {
.flags = PHY_HAS_INTERRUPT,
.probe = marvell_probe,
.config_init = &m88e1111_config_init,
- .config_aneg = &m88e1111_config_aneg,
+ .config_aneg = &marvell_config_aneg,
.read_status = &marvell_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/2] net: phy: Stop with excessive soft reset
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
In-Reply-To: <20180925182846.30042-1-f.fainelli@gmail.com>
While consolidating the PHY reset in phy_init_hw() an unconditionaly
BMCR soft-reset I became quite trigger happy with those. This was later
on deactivated for the Generic PHY driver on the premise that a prior
software entity (e.g: bootloader) might have applied workarounds in
commit 0878fff1f42c ("net: phy: Do not perform software reset for
Generic PHY").
Since we have a hook to wire-up a soft_reset callback, just use that and
get rid of the call to genphy_soft_reset() entirely. This speeds up
initialization and link establishment for most PHYs out there that do
not require a reset.
Fixes: 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()")
Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy_device.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index af64a9320fb0..ee676d75fe02 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -880,8 +880,6 @@ int phy_init_hw(struct phy_device *phydev)
if (phydev->drv->soft_reset)
ret = phydev->drv->soft_reset(phydev);
- else
- ret = genphy_soft_reset(phydev);
if (ret < 0)
return ret;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 0/2] net: phy: Eliminate unnecessary soft
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
Hi all,
This patch series eliminates unnecessary software resets of the PHY.
This should hopefully not break anybody's hardware; but I would
appreciate testing to make sure this is is the case.
Sorry for this long email list, I wanted to make sure I reached out to
all people who made changes to the Marvell PHY driver.
Thank you!
Changes since RFT:
- added Tested-by tags from Wang, Dongsheng, Andrew, Chris and Clemens
Florian Fainelli (2):
net: phy: Stop with excessive soft reset
net: phy: marvell: Avoid unnecessary soft reset
drivers/net/phy/marvell.c | 63 ++++++++++++------------------------
drivers/net/phy/phy_device.c | 2 --
2 files changed, 21 insertions(+), 44 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [RFC PATCH iproute2-next] System specification health API
From: Eran Ben Elisha @ 2018-09-25 12:17 UTC (permalink / raw)
To: Andrew Lunn, Stephen Hemminger
Cc: Jakub Kicinski, netdev, Jiri Pirko, Andy Gospodarek, Michael Chan,
Simon Horman, Alexander Duyck, Florian Fainelli, Tal Alon,
Ariel Almog
In-Reply-To: <20180916195727.GD19261@lunn.ch>
On 9/16/2018 10:57 PM, Andrew Lunn wrote:
>> Why is this going under iproute rather than using one of the existing sensor API's.
>> For example Intel NIC's have thermal sensors etc.
>
> Hi Stephen
>
> These are not that sort of sensors. This is part of the naming problem
> here. It is not really to do with health, it is about exceptions and
> bugs. And the sensors are more like timeouts and watchdogs.
>
> It is clear that the current names lead to a lot of confusion. Maybe:
>
> health -> exception
> sensor -> condition
>
> Andrew
>
I think those names renaming can work well.
(Sorry for that response, Local holiday season...)
Eran
^ permalink raw reply
* Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.
From: Stephen Hemminger @ 2018-09-25 12:07 UTC (permalink / raw)
To: Christian Brauner; +Cc: David Ahern, netdev@vger.kernel.org, David Miller
In-Reply-To: <20180925094908.yuu5bsrazkiq3ihy@brauner.io>
On Tue, 25 Sep 2018 11:49:10 +0200
Christian Brauner <christian@brauner.io> wrote:
> On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:
> > On top of net-next I am see a dmesg error:
> >
> > netlink: 16 bytes leftover after parsing attributes in process `ip'.
> >
> > I traced it to address lists and commit:
> >
> > commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> > Author: Christian Brauner <christian@brauner.io>
> > Date: Tue Sep 4 21:53:50 2018 +0200
> >
> > ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> >
> > Per the commit you are trying to guess whether the ancillary header is
> > an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong. :-)
>
> Well, I currently don't guess at all. :) I'm parsing with struct
> ifaddrmsg as assumed header size but ignore parsing errors when that
> fails. You don't get the niceties of the new property if you don't pack
There are legacy parts of netlink interface with kernel.
The ABI has evolved over time but some old parts are stuck in the past.
> > I don't have time to take this to ground, but address listing is not the
> > only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> > have thought about this for route dumps, but its solution does not work
> > here. You'll need to find something because the current warning on every
> > address dump is not acceptable.
>
> Two points before I propose a migitation:
>
> 1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
> userspace is doing something wrong is imho justifiable.
> Actually, I would argue that we should not hide the problem from
> userspace at all. The rate-limited (so no logging DOS afaict) warning
> messages are a perfect indicator that a tool is doing something wrong
> *without* introducing any regressions.
> The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
> be used too. Additionally, userspace stuffs an ifinfomsg in there but
> expects to receive ifaddrmsg. They should be warned loudly. :) So I
> actually like the warning messages.
> 2. Userspace should be fixed. Especially such an important standard tool
> as iproute2 that is maintained on git.kernel.org (glibc is already
> doing the right.).
>
> So if people really want to hide this issue as much as we can then we
> can play the guessing game. I could send a patch that roughly does the
> following:
>
> if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> guessed_header_len = sizeof(struct ifaddrmsg);
> else
> guessed_header_len = sizeof(struct ifinfomsg);
>
> This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> This propert is a __s32 which should bring the message up to 12 bytes
> (not sure about alignment requiremnts and where we might wend up ten)
> which is still less than the 16 bytes without that property from
> ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> break when ifaddrmsg grows a new member or we introduce another property
> that is valid in RTM_GETADDR requests. It also will not work cleanly
> when users stuff additional properties in there that are valid for the
> address family but are not used int RTM_GETADDR requests.
>
> I would like to hear what other people and davem think we should do.
> Patch it away or print the warning.
>
> Christian
You can't break old programs. That is one of the rules of kernel.
Therefore, please either revert the kernel change or put the new attribute
in a place where old versions do not cause problem.
There are people who run new kernels on old versions of iproute (like enterprise
distributions) and vice versa.
^ permalink raw reply
* Re: [RFC PATCH iproute2-next] System specification health API
From: Eran Ben Elisha @ 2018-09-25 12:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, Jiri Pirko, Andy Gospodarek, Michael Chan, Simon Horman,
Alexander Duyck, Andrew Lunn, Florian Fainelli, Tal Alon,
Ariel Almog
In-Reply-To: <fc30a529-6d66-c491-1024-85e68db77af7@mellanox.com>
On 9/16/2018 1:37 PM, Eran Ben Elisha wrote:
>
>
> On 9/13/2018 8:36 PM, Jakub Kicinski wrote:
>> On Thu, 13 Sep 2018 11:18:15 +0300, Eran Ben Elisha wrote:
>>> The health spec is targeted for Real Time Alerting, in order to know
>>> when
>>> something bad had happened to a PCI device
>>
>> By spec you mean some standards body spec you implement or this
>> proposal is a spec?
>
> This proposal is a spec
>
>>
>>> - Provide alert debug information
>>> - Self healing
>>> - If problem needs vendor support, provide a way to gather all needed
>>> debugging
>>> information.
>>>
>>> The health contains sensors which sense for malfunction. Once sensor
>>> triggered,
>>> actions such as logs and correction can be taken.
>>> Sensors are sensing the health state and can trigger correction action.
>>>
>>> The sensors are divided into the following groups
>>> - Hardware sensor - a sensor which is triggered by the device due to
>>> malfunction.
>>> - Software sensor - a sensor which is triggered by the software due to
>>> malfunction.
>>> Both group of sensors can be triggered due to error event or due to a
>>> periodic check.
>>>
>>> Actions are the way to handle sensor events. Action can be in one of the
>>> following groups:
>>> - Dump - SW trace, SW dump, HW trace, HW dump
>>> - Reset - Surgical correction (e.g. modify Q, flush Q, reset of
>>> device, etc)
>>> Actions can be performed by SW or HW.
>>>
>>> User is allowed to enable or disable sensors and sensor2action mapping.
>>>
>>> This RFC man page patch describes the suggested API of devlink-health
>>> in order
>>> to control sensors and actions.
>>
>> I like the idea of configuring response to events like this, although
>> I'm not sure the name sensor is appropriate here - perhaps exception or
>> error would be better?
>
> I was trying to avoid the negativity description. Have it called sensor
> to avoid restricting the API for errors / exceptions only. I got the
> same type of comment from Andrew as well devlink-health->devlink-bug.
>
> But if other vendors driver developers don't see it can be expanded to
> sensor which are not errors, then I guess we can refactor the names.
>
> Are there going to be values reported?
>
> It depends on the sensor. If it has data that would help in the debug,
> then I assume yes, via the dumps.
>
>>
>> I'm not so sure about HW sensors in relation to existing HWMON
>> infrastructure... I assume you're targeting things like say some HW
>> engine/block reporting it encountered an error? Sounds good, too.
>
> yes, exactly.
>
>>
>> Are the actions all envisioned to be performed by the driver?
>> Firmware? Hardware? I guess that distinction can be added later.
>> For FW/HW actions we would go back to the problem of persistence of
>> the setting since it was only implemented for params :S
>
> The problem is not with FW action, the problem is when you try to set
> sensor2action mapping for the FW/HW. this will need persistence
> configuration mode. Sensor2action in SW shall be run-time mode (at least
> as a start).
> But it sound as this need some more tuning, to make it clear.
Revisiting this (before sending V2). My guideline is that persistency
inside the device is needed only when a persistence information is
needed before the driver loads. For any other configuration (i.e post HW
boot), one can use standard Linux scripts in order to control its
persistence information.
If any new sensor will be added that requires pre HW boot information,
the API can be extended later.
>
>>
>> Is the dump option going to tie back into region snapshots?
>>
> no necessarily, dumping SW objects as well can be helpful
^ permalink raw reply
* Re: bpfilter breaks IPT_SO_GET_INFO
From: Dmitry Vyukov @ 2018-09-25 17:59 UTC (permalink / raw)
To: Michal Kubecek
Cc: Alexei Starovoitov, David Miller, Daniel Borkmann, netdev, LKML,
syzkaller, NetFilter, Fabian Vogt, Takashi Iwai
In-Reply-To: <CACT4Y+bdzDe_LhQQh-eLapOg0ZsfXFRm2tjUbiAsgS-ObMvwBg@mail.gmail.com>
On Wed, Sep 19, 2018 at 10:29 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Sep 19, 2018 at 9:18 AM, Michal Kubecek <mkubecek@suse.cz> wrote:
>> On Mon, Sep 17, 2018 at 03:36:21PM +0200, Dmitry Vyukov wrote:
>>> Hi,
>>>
>>> I am having some problem with upstream kernel and bpfilter. The
>>> manifestation is that IPT_SO_GET_INFO on an ipv4 socket works, then
>>> something (that I can't fully localize but can reproduce) happens and
>>> then IPT_SO_GET_INFO starts permanently returning 256.
>> ...
>>> Now the litmus program always fails with:
>>>
>>> getsockopt(3, SOL_IP, 0x40 /* IP_??? */,
>>> "filter\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., [84])
>>> = 256
>>>
>>> I am currently on upstream commit
>>> 28619527b8a712590c93d0a9e24b4425b9376a8c, my .config is attached. I
>>> don't know what is bpfilter, I see it mentions some umh, if it
>>> requires some additional setup I don't it, i.e. I don't install any
>>> userspace modules/helpers.
>>
>> This looks similar to the fallback issue described here:
>>
>> https://bugzilla.suse.com/show_bug.cgi?id=1106751#c1
>>
>> Unfortunately I didn't have time to look into it more closely yet.
>
> +Takashi
>
> But I already have CONFIG_BPFILTER_UMH=y in my config, so it does not
> help completely.
> Also in my case it is working initially, but breaks after I run the
> second program.
I've disabled CONFIG_BPFILTER for now, it causes too many failures.
https://github.com/google/syzkaller/commit/19a403430d8d5ae2472e16dab2f26ddd899cf552
^ permalink raw reply
* Re: [PATCH net] net: hns: fix for unmapping problem when SMMU is on
From: David Miller @ 2018-09-25 17:43 UTC (permalink / raw)
To: salil.mehta
Cc: yisen.zhuang, lipeng321, mehta.salil, netdev, linux-kernel,
linuxarm, linyunsheng
In-Reply-To: <20180925092155.11024-1-salil.mehta@huawei.com>
From: Salil Mehta <salil.mehta@huawei.com>
Date: Tue, 25 Sep 2018 10:21:55 +0100
> From: Yunsheng Lin <linyunsheng@huawei.com>
>
> If SMMU is on, there is more likely that skb_shinfo(skb)->frags[i]
> can not send by a single BD. when this happen, the
> hns_nic_net_xmit_hw function map the whole data in a frags using
> skb_frag_dma_map, but unmap each BD' data individually when tx is
> done, which causes problem when SMMU is on.
>
> This patch fixes this problem by ummapping the whole data in a
> frags when tx is done.
>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Reviewed-by: Yisen Zhuang <yisen.zhuang@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH v2] net: macb: Clean 64b dma addresses if they are not detected
From: David Miller @ 2018-09-25 17:37 UTC (permalink / raw)
To: michal.simek; +Cc: linux-kernel, monstr, edgar.iglesias, netdev, nicolas.ferre
In-Reply-To: <68051905ba59d7cfd74c63ef5bf0830dc2b9f6fe.1537857166.git.michal.simek@xilinx.com>
From: Michal Simek <michal.simek@xilinx.com>
Date: Tue, 25 Sep 2018 08:32:50 +0200
> Clear ADDR64 dma bit in DMACFG register in case that HW_DMA_CAP_64B is
> not detected on 64bit system.
> The issue was observed when bootloader(u-boot) does not check macb
> feature at DCFG6 register (DAW64_OFFSET) and enabling 64bit dma support
> by default. Then macb driver is reading DMACFG register back and only
> adding 64bit dma configuration but not cleaning it out.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v2:
> - Clean reg at the first place - Edgar
> - Update commit message
Applied, thank you.
^ permalink raw reply
* RE: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
From: Woojung.Huh @ 2018-09-25 17:32 UTC (permalink / raw)
To: f.fainelli, netdev
Cc: davem, UNGLinuxDriver, steve.glendinning, keescook, akurz,
hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <f791e4b6-7493-b54b-a88f-47d0ef4a3409@gmail.com>
Hi Florian,
> >> + if (pdata->wol == 0)
> >> + return -EINVAL;
> >> +
> > It will make function return when disabling WOL.
>
> Huh, yes, good point.
>
> > Is there other place handling this scenario?
>
> How do you mean?
>
I meant there is another path I might miss when disabling WOL
than this xxx_set_wol().
Thanks
Woojung
^ permalink raw reply
* Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
From: Florian Fainelli @ 2018-09-25 17:26 UTC (permalink / raw)
To: Woojung.Huh, netdev
Cc: davem, UNGLinuxDriver, steve.glendinning, keescook, akurz,
hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <BN6PR1101MB2130115311A87CCF7D712B0FE7160@BN6PR1101MB2130.namprd11.prod.outlook.com>
On 09/25/2018 10:19 AM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
>
>> @@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev,
>> if (wol->wolopts & WAKE_ARP)
>> pdata->wol |= WAKE_ARP;
>>
>> + if (pdata->wol == 0)
>> + return -EINVAL;
>> +
> It will make function return when disabling WOL.
Huh, yes, good point.
> Is there other place handling this scenario?
How do you mean?
>
>> device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
>>
>> phy_ethtool_set_wol(netdev->phydev, wol);
>
>
> Thanks.
> Woojung
>
--
Florian
^ permalink raw reply
* RE: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
From: Woojung.Huh @ 2018-09-25 17:19 UTC (permalink / raw)
To: f.fainelli, netdev
Cc: davem, UNGLinuxDriver, steve.glendinning, keescook, akurz,
hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <20180924205420.31309-4-f.fainelli@gmail.com>
Hi Florian,
> @@ -1415,6 +1415,9 @@ static int lan78xx_set_wol(struct net_device *netdev,
> if (wol->wolopts & WAKE_ARP)
> pdata->wol |= WAKE_ARP;
>
> + if (pdata->wol == 0)
> + return -EINVAL;
> +
It will make function return when disabling WOL.
Is there other place handling this scenario?
> device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
>
> phy_ethtool_set_wol(netdev->phydev, wol);
Thanks.
Woojung
^ permalink raw reply
* [PATCH net-next] tls: Fix socket mem accounting error under async encryption
From: Vakul Garg @ 2018-09-25 10:56 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, doronrk, Vakul Garg
Current async encryption implementation sometimes showed up socket
memory accounting error during socket close. This results in kernel
warning calltrace. The root cause of the problem is that socket var
sk_forward_alloc gets corrupted due to access in sk_mem_charge()
and sk_mem_uncharge() being invoked from multiple concurrent contexts
in multicore processor. The apis sk_mem_charge() and sk_mem_uncharge()
are called from functions alloc_plaintext_sg(), free_sg() etc. It is
required that memory accounting apis are called under a socket lock.
The plaintext sg data sent for encryption is freed using free_sg() in
tls_encryption_done(). It is wrong to call free_sg() from this function.
This is because this function may run in irq context. We cannot acquire
socket lock in this function.
We remove calling of function free_sg() for plaintext data from
tls_encryption_done() and defer freeing up of plaintext data to the time
when the record is picked up from tx_list and transmitted/freed. When
tls_tx_records() gets called, socket is already locked and thus there is
no concurrent access problem.
Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption")
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
net/tls/tls_sw.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index bf03f32aa983..406d3bb98818 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -353,6 +353,9 @@ int tls_tx_records(struct sock *sk, int flags)
* Remove the head of tx_list
*/
list_del(&rec->list);
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
+
kfree(rec);
}
@@ -371,6 +374,10 @@ int tls_tx_records(struct sock *sk, int flags)
goto tx_err;
list_del(&rec->list);
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size);
+
kfree(rec);
} else {
break;
@@ -399,8 +406,6 @@ static void tls_encrypt_done(struct crypto_async_request *req, int err)
rec->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
- free_sg(sk, rec->sg_plaintext_data,
- &rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
/* Free the record if error is previously set on socket */
if (err || sk->sk_err) {
@@ -523,9 +528,6 @@ static int tls_push_record(struct sock *sk, int flags,
if (rc == -EINPROGRESS)
return -EINPROGRESS;
- free_sg(sk, rec->sg_plaintext_data, &rec->sg_plaintext_num_elem,
- &rec->sg_plaintext_size);
-
if (rc < 0) {
tls_err_abort(sk, EBADMSG);
return rc;
@@ -1566,6 +1568,11 @@ void tls_sw_free_resources_tx(struct sock *sk)
rec = list_first_entry(&ctx->tx_list,
struct tls_rec, list);
+
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size);
+
list_del(&rec->list);
kfree(rec);
}
@@ -1575,6 +1582,10 @@ void tls_sw_free_resources_tx(struct sock *sk)
&rec->sg_encrypted_num_elem,
&rec->sg_encrypted_size);
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size);
+
list_del(&rec->list);
kfree(rec);
}
--
2.13.6
^ permalink raw reply related
* Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.
From: Christian Brauner @ 2018-09-25 9:49 UTC (permalink / raw)
To: David Ahern; +Cc: netdev@vger.kernel.org, David Miller
In-Reply-To: <6059bf4e-b1cf-2c7e-5529-9003bdd8a14b@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]
On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:
> On top of net-next I am see a dmesg error:
>
> netlink: 16 bytes leftover after parsing attributes in process `ip'.
>
> I traced it to address lists and commit:
>
> commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> Author: Christian Brauner <christian@brauner.io>
> Date: Tue Sep 4 21:53:50 2018 +0200
>
> ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
>
> Per the commit you are trying to guess whether the ancillary header is
> an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong. :-)
Well, I currently don't guess at all. :) I'm parsing with struct
ifaddrmsg as assumed header size but ignore parsing errors when that
fails. You don't get the niceties of the new property if you don't pack
it up nicely in an ifaddrmsg struct. :)
>
> I don't have time to take this to ground, but address listing is not the
> only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> have thought about this for route dumps, but its solution does not work
> here. You'll need to find something because the current warning on every
> address dump is not acceptable.
Two points before I propose a migitation:
1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
userspace is doing something wrong is imho justifiable.
Actually, I would argue that we should not hide the problem from
userspace at all. The rate-limited (so no logging DOS afaict) warning
messages are a perfect indicator that a tool is doing something wrong
*without* introducing any regressions.
The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
be used too. Additionally, userspace stuffs an ifinfomsg in there but
expects to receive ifaddrmsg. They should be warned loudly. :) So I
actually like the warning messages.
2. Userspace should be fixed. Especially such an important standard tool
as iproute2 that is maintained on git.kernel.org (glibc is already
doing the right.).
So if people really want to hide this issue as much as we can then we
can play the guessing game. I could send a patch that roughly does the
following:
if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
guessed_header_len = sizeof(struct ifaddrmsg);
else
guessed_header_len = sizeof(struct ifinfomsg);
This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
This propert is a __s32 which should bring the message up to 12 bytes
(not sure about alignment requiremnts and where we might wend up ten)
which is still less than the 16 bytes without that property from
ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
break when ifaddrmsg grows a new member or we introduce another property
that is valid in RTM_GETADDR requests. It also will not work cleanly
when users stuff additional properties in there that are valid for the
address family but are not used int RTM_GETADDR requests.
I would like to hear what other people and davem think we should do.
Patch it away or print the warning.
Christian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v3 1/2] netlink: ipv4 igmp join notifications
From: Patrick Ruddy @ 2018-09-25 9:34 UTC (permalink / raw)
To: David Ahern, Roopa Prabhu
Cc: netdev, Jiří Pírko, Stephen Hemminger,
Nikolay Aleksandrov
In-Reply-To: <43d8f45e-64f6-17a6-d07f-99c33a515347@gmail.com>
On Wed, 2018-09-19 at 21:47 -0700, David Ahern wrote:
> On 9/18/18 6:12 AM, Patrick Ruddy wrote:
> >
> > I've hit a small snag with adding the new groups. The number of defined
> > groups currently sits at 31 so I can only add one before hitting the
>
> I believe you have no more available. RTNLGRP_* has been defined from 0
> (RTNLGRP_NONE) to 31 (RTNLGRP_IPV6_MROUTE_R) which covers the u32 range.
>
> > limit defined by the 32 bit groups bitmask in socakddr_nl. I can use 1
> > group for both v4 and v6 notifications which seems like the sensible
> > options since the AF is carried separately, but it breaks the precedent
> > where there are separate IPV4 and IPV6 groups for IFADDR.
> >
> > I have the combined group patches ready and can share them if that's
> > the preference.
> >
> > Has there been any previous discussion about extending the number of
> > availabel groups?
> >
>
> I have not tried it, but from a prior code review I believe you have you
> use setsockopt to add groups > 31.
I can certainly join the new groups using setsockopt and
NETLINK_ADD_MEMBERSHIP.
I can't see any examples of extending the defined group list within the
kernel so I assume I just add to the RTNLGRP enum list with a suitable
comment to indicate that later groups must be joined with the mechanism
above or am I missing some other way of dynamically adding groups?
thanks
-pr
^ 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