* [PATCH v3 1/2] bpf: add a longest prefix match trie map implementation
From: Daniel Mack @ 2017-01-14 12:17 UTC (permalink / raw)
To: ast; +Cc: dh.herrmann, daniel, netdev, davem, Daniel Mack
In-Reply-To: <20170114121727.14784-1-daniel@zonque.org>
This trie implements a longest prefix match algorithm that can be used
to match IP addresses to a stored set of ranges.
Internally, data is stored in an unbalanced trie of nodes that has a
maximum height of n, where n is the prefixlen the trie was created
with.
Tries may be created with prefix lengths that are multiples of 8, in
the range from 8 to 2048. The key used for lookup and update operations
is a struct bpf_lpm_trie_key, and the value is a uint64_t.
The code carries more information about the internal implementation.
Signed-off-by: Daniel Mack <daniel@zonque.org>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
---
include/uapi/linux/bpf.h | 7 +
kernel/bpf/Makefile | 2 +-
kernel/bpf/lpm_trie.c | 493 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 501 insertions(+), 1 deletion(-)
create mode 100644 kernel/bpf/lpm_trie.c
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0eb0e87..d564277 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -63,6 +63,12 @@ struct bpf_insn {
__s32 imm; /* signed immediate constant */
};
+/* Key of an a BPF_MAP_TYPE_LPM_TRIE entry */
+struct bpf_lpm_trie_key {
+ __u32 prefixlen; /* up to 32 for AF_INET, 128 for AF_INET6 */
+ __u8 data[0]; /* Arbitrary size */
+};
+
/* BPF syscall commands, see bpf(2) man-page for details. */
enum bpf_cmd {
BPF_MAP_CREATE,
@@ -89,6 +95,7 @@ enum bpf_map_type {
BPF_MAP_TYPE_CGROUP_ARRAY,
BPF_MAP_TYPE_LRU_HASH,
BPF_MAP_TYPE_LRU_PERCPU_HASH,
+ BPF_MAP_TYPE_LPM_TRIE,
};
enum bpf_prog_type {
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 1276474..e1ce4f4 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -1,7 +1,7 @@
obj-y := core.o
obj-$(CONFIG_BPF_SYSCALL) += syscall.o verifier.o inode.o helpers.o
-obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o
+obj-$(CONFIG_BPF_SYSCALL) += hashtab.o arraymap.o percpu_freelist.o bpf_lru_list.o lpm_trie.o
ifeq ($(CONFIG_PERF_EVENTS),y)
obj-$(CONFIG_BPF_SYSCALL) += stackmap.o
endif
diff --git a/kernel/bpf/lpm_trie.c b/kernel/bpf/lpm_trie.c
new file mode 100644
index 0000000..1c1ad27
--- /dev/null
+++ b/kernel/bpf/lpm_trie.c
@@ -0,0 +1,493 @@
+/*
+ * Longest prefix match list implementation
+ *
+ * Copyright (c) 2016,2017 Daniel Mack
+ * Copyright (c) 2016 David Herrmann
+ *
+ * This file is subject to the terms and conditions of version 2 of the GNU
+ * General Public License. See the file COPYING in the main directory of the
+ * Linux distribution for more details.
+ */
+
+#include <linux/bpf.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/vmalloc.h>
+#include <net/ipv6.h>
+
+/* Intermediate node */
+#define LPM_TREE_NODE_FLAG_IM BIT(0)
+
+struct lpm_trie_node;
+
+struct lpm_trie_node {
+ struct rcu_head rcu;
+ struct lpm_trie_node __rcu *child[2];
+ u32 prefixlen;
+ u32 flags;
+ u8 data[0];
+};
+
+struct lpm_trie {
+ struct bpf_map map;
+ struct lpm_trie_node __rcu *root;
+ size_t n_entries;
+ size_t max_prefixlen;
+ size_t data_size;
+ raw_spinlock_t lock;
+};
+
+/* This trie implements a longest prefix match algorithm that can be used to
+ * match IP addresses to a stored set of ranges.
+ *
+ * Data stored in @data of struct bpf_lpm_key and struct lpm_trie_node is
+ * interpreted as big endian, so data[0] stores the most significant byte.
+ *
+ * Match ranges are internally stored in instances of struct lpm_trie_node
+ * which each contain their prefix length as well as two pointers that may
+ * lead to more nodes containing more specific matches. Each node also stores
+ * a value that is defined by and returned to userspace via the update_elem
+ * and lookup functions.
+ *
+ * For instance, let's start with a trie that was created with a prefix length
+ * of 32, so it can be used for IPv4 addresses, and one single element that
+ * matches 192.168.0.0/16. The data array would hence contain
+ * [0xc0, 0xa8, 0x00, 0x00] in big-endian notation. This documentation will
+ * stick to IP-address notation for readability though.
+ *
+ * As the trie is empty initially, the new node (1) will be places as root
+ * node, denoted as (R) in the example below. As there are no other node, both
+ * child pointers are %NULL.
+ *
+ * +----------------+
+ * | (1) (R) |
+ * | 192.168.0.0/16 |
+ * | value: 1 |
+ * | [0] [1] |
+ * +----------------+
+ *
+ * Next, let's add a new node (2) matching 192.168.0.0/24. As there is already
+ * a node with the same data and a smaller prefix (ie, a less specific one),
+ * node (2) will become a child of (1). In child index depends on the next bit
+ * that is outside of what (1) matches, and that bit is 0, so (2) will be
+ * child[0] of (1):
+ *
+ * +----------------+
+ * | (1) (R) |
+ * | 192.168.0.0/16 |
+ * | value: 1 |
+ * | [0] [1] |
+ * +----------------+
+ * |
+ * +----------------+
+ * | (2) |
+ * | 192.168.0.0/24 |
+ * | value: 2 |
+ * | [0] [1] |
+ * +----------------+
+ *
+ * The child[1] slot of (1) could be filled with another node which has bit #17
+ * (the next bit after the ones that (1) matches on) set to 1. For instance,
+ * 192.168.128.0/24:
+ *
+ * +----------------+
+ * | (1) (R) |
+ * | 192.168.0.0/16 |
+ * | value: 1 |
+ * | [0] [1] |
+ * +----------------+
+ * | |
+ * +----------------+ +------------------+
+ * | (2) | | (3) |
+ * | 192.168.0.0/24 | | 192.168.128.0/24 |
+ * | value: 2 | | value: 3 |
+ * | [0] [1] | | [0] [1] |
+ * +----------------+ +------------------+
+ *
+ * Let's add another node (4) to the game for 192.168.1.0/24. In order to place
+ * it, node (1) is looked at first, and because (4) of the semantics laid out
+ * above (bit #17 is 0), it would normally be attached to (1) as child[0].
+ * However, that slot is already allocated, so a new node is needed in between.
+ * That node does not have a value attached to it and it will never be
+ * returned to users as result of a lookup. It is only there to differentiate
+ * the traversal further. It will get a prefix as wide as necessary to
+ * distinguish its two children:
+ *
+ * +----------------+
+ * | (1) (R) |
+ * | 192.168.0.0/16 |
+ * | value: 1 |
+ * | [0] [1] |
+ * +----------------+
+ * | |
+ * +----------------+ +------------------+
+ * | (4) (I) | | (3) |
+ * | 192.168.0.0/23 | | 192.168.128.0/24 |
+ * | value: --- | | value: 3 |
+ * | [0] [1] | | [0] [1] |
+ * +----------------+ +------------------+
+ * | |
+ * +----------------+ +----------------+
+ * | (2) | | (5) |
+ * | 192.168.0.0/24 | | 192.168.1.0/24 |
+ * | value: 2 | | value: 5 |
+ * | [0] [1] | | [0] [1] |
+ * +----------------+ +----------------+
+ *
+ * 192.168.1.1/32 would be a child of (5) etc.
+ *
+ * An intermediate node will be turned into a 'real' node on demand. In the
+ * example above, (4) would be re-used if 192.168.0.0/23 is added to the trie.
+ *
+ * A fully populated trie would have a height of 32 nodes, as the trie was
+ * created with a prefix length of 32.
+ *
+ * The lookup starts at the root node. If the current node matches and if there
+ * is a child that can be used to become more specific, the trie is traversed
+ * downwards. The last node in the traversal that is a non-intermediate one is
+ * returned.
+ */
+
+static inline int extract_bit(const u8 *data, size_t index)
+{
+ return !!(data[index / 8] & (1 << (7 - (index % 8))));
+}
+
+/**
+ * longest_prefix_match() - determine the longest prefix
+ * @trie: The trie to get internal sizes from
+ * @node: The node to operate on
+ * @key: The key to compare to @node
+ *
+ * Determine the longest prefix of @node that matches the bits in @key.
+ */
+static size_t longest_prefix_match(const struct lpm_trie *trie,
+ const struct lpm_trie_node *node,
+ const struct bpf_lpm_trie_key *key)
+{
+ size_t prefixlen = 0;
+ size_t i;
+
+ for (i = 0; i < trie->data_size; i++) {
+ size_t b;
+
+ b = 8 - fls(node->data[i] ^ key->data[i]);
+ prefixlen += b;
+
+ if (prefixlen >= node->prefixlen || prefixlen >= key->prefixlen)
+ return min(node->prefixlen, key->prefixlen);
+
+ if (b < 8)
+ break;
+ }
+
+ return prefixlen;
+}
+
+/* Called from syscall or from eBPF program */
+static void *trie_lookup_elem(struct bpf_map *map, void *_key)
+{
+ struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
+ struct lpm_trie_node *node, *found = NULL;
+ struct bpf_lpm_trie_key *key = _key;
+
+ /* Start walking the trie from the root node ... */
+
+ for (node = rcu_dereference(trie->root); node;) {
+ unsigned int next_bit;
+ size_t matchlen;
+
+ /* Determine the longest prefix of @node that matches @key.
+ * If it's the maximum possible prefix for this trie, we have
+ * an exact match and can return it directly.
+ */
+ matchlen = longest_prefix_match(trie, node, key);
+ if (matchlen == trie->max_prefixlen) {
+ found = node;
+ break;
+ }
+
+ /* If the number of bits that match is smaller than the prefix
+ * length of @node, bail out and return the node we have seen
+ * last in the traversal (ie, the parent).
+ */
+ if (matchlen < node->prefixlen)
+ break;
+
+ /* Consider this node as return candidate unless it is an
+ * artificially added intermediate one.
+ */
+ if (!(node->flags & LPM_TREE_NODE_FLAG_IM))
+ found = node;
+
+ /* If the node match is fully satisfied, let's see if we can
+ * become more specific. Determine the next bit in the key and
+ * traverse down.
+ */
+ next_bit = extract_bit(key->data, node->prefixlen);
+ node = rcu_dereference(node->child[next_bit]);
+ }
+
+ if (!found)
+ return NULL;
+
+ return found->data + trie->data_size;
+}
+
+static struct lpm_trie_node *lpm_trie_node_alloc(const struct lpm_trie *trie,
+ const void *value)
+{
+ struct lpm_trie_node *node;
+ size_t size = sizeof(struct lpm_trie_node) + trie->data_size;
+
+ if (value)
+ size += trie->map.value_size;
+
+ node = kmalloc(size, GFP_ATOMIC | __GFP_NOWARN);
+ if (!node)
+ return NULL;
+
+ node->flags = 0;
+
+ if (value)
+ memcpy(node->data + trie->data_size, value,
+ trie->map.value_size);
+
+ return node;
+}
+
+/* Called from syscall or from eBPF program */
+static int trie_update_elem(struct bpf_map *map,
+ void *_key, void *value, u64 flags)
+{
+ struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
+ struct lpm_trie_node *node, *im_node, *new_node = NULL;
+ struct lpm_trie_node __rcu **slot;
+ struct bpf_lpm_trie_key *key = _key;
+ unsigned long irq_flags;
+ unsigned int next_bit;
+ size_t matchlen = 0;
+ int ret = 0;
+
+ if (unlikely(flags > BPF_EXIST))
+ return -EINVAL;
+
+ if (key->prefixlen > trie->max_prefixlen)
+ return -EINVAL;
+
+ raw_spin_lock_irqsave(&trie->lock, irq_flags);
+
+ /* Allocate and fill a new node */
+
+ if (trie->n_entries == trie->map.max_entries) {
+ ret = -ENOSPC;
+ goto out;
+ }
+
+ new_node = lpm_trie_node_alloc(trie, value);
+ if (!new_node) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ trie->n_entries++;
+
+ new_node->prefixlen = key->prefixlen;
+ RCU_INIT_POINTER(new_node->child[0], NULL);
+ RCU_INIT_POINTER(new_node->child[1], NULL);
+ memcpy(new_node->data, key->data, trie->data_size);
+
+ /* Now find a slot to attach the new node. To do that, walk the tree
+ * from the root and match as many bits as possible for each node until
+ * we either find an empty slot or a slot that needs to be replaced by
+ * an intermediate node.
+ */
+ slot = &trie->root;
+
+ while ((node = rcu_dereference_protected(*slot,
+ lockdep_is_held(&trie->lock)))) {
+ matchlen = longest_prefix_match(trie, node, key);
+
+ if (node->prefixlen != matchlen ||
+ node->prefixlen == key->prefixlen ||
+ node->prefixlen == trie->max_prefixlen)
+ break;
+
+ next_bit = extract_bit(key->data, node->prefixlen);
+ slot = &node->child[next_bit];
+ }
+
+ /* If the slot is empty (a free child pointer or an empty root),
+ * simply assign the @new_node to that slot and be done.
+ */
+ if (!node) {
+ rcu_assign_pointer(*slot, new_node);
+ goto out;
+ }
+
+ /* If the slot we picked already exists, replace it with @new_node
+ * which already has the correct data array set.
+ */
+ if (node->prefixlen == matchlen) {
+ new_node->child[0] = node->child[0];
+ new_node->child[1] = node->child[1];
+
+ if (!(node->flags & LPM_TREE_NODE_FLAG_IM))
+ trie->n_entries--;
+
+ rcu_assign_pointer(*slot, new_node);
+ kfree_rcu(node, rcu);
+
+ goto out;
+ }
+
+ /* If the new node matches the prefix completely, it must be inserted
+ * as an ancestor. Simply insert it between @node and *@slot.
+ */
+ if (matchlen == key->prefixlen) {
+ next_bit = extract_bit(node->data, matchlen);
+ rcu_assign_pointer(new_node->child[next_bit], node);
+ rcu_assign_pointer(*slot, new_node);
+ goto out;
+ }
+
+ im_node = lpm_trie_node_alloc(trie, NULL);
+ if (!im_node) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ im_node->prefixlen = matchlen;
+ im_node->flags |= LPM_TREE_NODE_FLAG_IM;
+ memcpy(im_node->data, node->data, trie->data_size);
+
+ /* Now determine which child to install in which slot */
+ if (extract_bit(key->data, matchlen)) {
+ rcu_assign_pointer(im_node->child[0], node);
+ rcu_assign_pointer(im_node->child[1], new_node);
+ } else {
+ rcu_assign_pointer(im_node->child[0], new_node);
+ rcu_assign_pointer(im_node->child[1], node);
+ }
+
+ /* Finally, assign the intermediate node to the determined spot */
+ rcu_assign_pointer(*slot, im_node);
+
+out:
+ if (ret) {
+ if (new_node)
+ trie->n_entries--;
+
+ kfree(new_node);
+ kfree(im_node);
+ }
+
+ raw_spin_unlock_irqrestore(&trie->lock, irq_flags);
+
+ return ret;
+}
+
+static struct bpf_map *trie_alloc(union bpf_attr *attr)
+{
+ size_t cost, cost_per_node;
+ struct lpm_trie *trie;
+ int ret;
+
+ /* check sanity of attributes */
+ if (attr->max_entries == 0 ||
+ attr->map_flags != BPF_F_NO_PREALLOC ||
+ attr->key_size < sizeof(struct bpf_lpm_trie_key) + 1 ||
+ attr->key_size > sizeof(struct bpf_lpm_trie_key) + 256 ||
+ attr->value_size == 0)
+ return ERR_PTR(-EINVAL);
+
+ trie = kzalloc(sizeof(*trie), GFP_USER | __GFP_NOWARN);
+ if (!trie)
+ return ERR_PTR(-ENOMEM);
+
+ /* copy mandatory map attributes */
+ trie->map.map_type = attr->map_type;
+ trie->map.key_size = attr->key_size;
+ trie->map.value_size = attr->value_size;
+ trie->map.max_entries = attr->max_entries;
+ trie->data_size = attr->key_size -
+ offsetof(struct bpf_lpm_trie_key, data);
+ trie->max_prefixlen = trie->data_size * 8;
+
+ cost_per_node = sizeof(struct lpm_trie_node) +
+ attr->value_size + trie->data_size;
+ cost = sizeof(*trie) + attr->max_entries * cost_per_node;
+ trie->map.pages = round_up(cost, PAGE_SIZE) >> PAGE_SHIFT;
+
+ ret = bpf_map_precharge_memlock(trie->map.pages);
+ if (ret) {
+ kfree(trie);
+ return ERR_PTR(ret);
+ }
+
+ raw_spin_lock_init(&trie->lock);
+
+ return &trie->map;
+}
+
+static void trie_free(struct bpf_map *map)
+{
+ struct lpm_trie *trie = container_of(map, struct lpm_trie, map);
+ struct lpm_trie_node __rcu **slot;
+ struct lpm_trie_node *node;
+
+ raw_spin_lock(&trie->lock);
+
+ /* Always start at the root and walk down to a node that has no
+ * children. Then free that node, nullify its reference in the parent
+ * and start over.
+ */
+
+ for (;;) {
+ slot = &trie->root;
+
+ for (;;) {
+ node = rcu_dereference_protected(*slot,
+ lockdep_is_held(&trie->lock));
+ if (!node)
+ goto unlock;
+
+ if (rcu_access_pointer(node->child[0])) {
+ slot = &node->child[0];
+ continue;
+ }
+
+ if (rcu_access_pointer(node->child[1])) {
+ slot = &node->child[1];
+ continue;
+ }
+
+ kfree(node);
+ RCU_INIT_POINTER(*slot, NULL);
+ break;
+ }
+ }
+
+unlock:
+ raw_spin_unlock(&trie->lock);
+}
+
+static const struct bpf_map_ops trie_ops = {
+ .map_alloc = trie_alloc,
+ .map_free = trie_free,
+ .map_lookup_elem = trie_lookup_elem,
+ .map_update_elem = trie_update_elem,
+};
+
+static struct bpf_map_type_list trie_type __read_mostly = {
+ .ops = &trie_ops,
+ .type = BPF_MAP_TYPE_LPM_TRIE,
+};
+
+static int __init register_trie_map(void)
+{
+ bpf_register_map_type(&trie_type);
+ return 0;
+}
+late_initcall(register_trie_map);
--
2.9.3
^ permalink raw reply related
* [PATCH v3 2/2] bpf: Add tests for the lpm trie map
From: Daniel Mack @ 2017-01-14 12:17 UTC (permalink / raw)
To: ast; +Cc: dh.herrmann, daniel, netdev, davem, Daniel Mack
In-Reply-To: <20170114121727.14784-1-daniel@zonque.org>
From: David Herrmann <dh.herrmann@gmail.com>
The first part of this program runs randomized tests against the
lpm-bpf-map. It implements a "Trivial Longest Prefix Match" (tlpm)
based on simple, linear, single linked lists. The implementation
should be pretty straightforward.
Based on tlpm, this inserts randomized data into bpf-lpm-maps and
verifies the trie-based bpf-map implementation behaves the same way
as tlpm.
The second part uses 'real world' IPv4 and IPv6 addresses and tests
the trie with those.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Mack <daniel@zonque.org>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_lpm_map.c | 358 +++++++++++++++++++++++++++++
3 files changed, 361 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_lpm_map.c
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 071431b..d3b1c9b 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -1,3 +1,4 @@
test_verifier
test_maps
test_lru_map
+test_lpm_map
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 7a5f245..064a3e5 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -1,8 +1,8 @@
CFLAGS += -Wall -O2 -I../../../../usr/include
-test_objs = test_verifier test_maps test_lru_map
+test_objs = test_verifier test_maps test_lru_map test_lpm_map
-TEST_PROGS := test_verifier test_maps test_lru_map test_kmod.sh
+TEST_PROGS := test_verifier test_maps test_lru_map test_lpm_map test_kmod.sh
TEST_FILES := $(test_objs)
all: $(test_objs)
diff --git a/tools/testing/selftests/bpf/test_lpm_map.c b/tools/testing/selftests/bpf/test_lpm_map.c
new file mode 100644
index 0000000..dd83f0b
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_lpm_map.c
@@ -0,0 +1,358 @@
+/*
+ * Randomized tests for eBPF longest-prefix-match maps
+ *
+ * This program runs randomized tests against the lpm-bpf-map. It implements a
+ * "Trivial Longest Prefix Match" (tlpm) based on simple, linear, singly linked
+ * lists. The implementation should be pretty straightforward.
+ *
+ * Based on tlpm, this inserts randomized data into bpf-lpm-maps and verifies
+ * the trie-based bpf-map implementation behaves the same way as tlpm.
+ */
+
+#include <assert.h>
+#include <errno.h>
+#include <inttypes.h>
+#include <linux/bpf.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <unistd.h>
+#include <arpa/inet.h>
+#include <sys/time.h>
+#include <sys/resource.h>
+
+#include "bpf_sys.h"
+#include "bpf_util.h"
+
+struct tlpm_node {
+ struct tlpm_node *next;
+ size_t n_bits;
+ uint8_t key[];
+};
+
+static struct tlpm_node *tlpm_add(struct tlpm_node *list,
+ const uint8_t *key,
+ size_t n_bits)
+{
+ struct tlpm_node *node;
+ size_t n;
+
+ /* add new entry with @key/@n_bits to @list and return new head */
+
+ n = (n_bits + 7) / 8;
+ node = malloc(sizeof(*node) + n);
+ assert(node);
+
+ node->next = list;
+ node->n_bits = n_bits;
+ memcpy(node->key, key, n);
+
+ return node;
+}
+
+static void tlpm_clear(struct tlpm_node *list)
+{
+ struct tlpm_node *node;
+
+ /* free all entries in @list */
+
+ while ((node = list)) {
+ list = list->next;
+ free(node);
+ }
+}
+
+static struct tlpm_node *tlpm_match(struct tlpm_node *list,
+ const uint8_t *key,
+ size_t n_bits)
+{
+ struct tlpm_node *best = NULL;
+ size_t i;
+
+ /*
+ * Perform longest prefix-match on @key/@n_bits. That is, iterate all
+ * entries and match each prefix against @key. Remember the "best"
+ * entry we find (i.e., the longest prefix that matches) and return it
+ * to the caller when done.
+ */
+
+ for ( ; list; list = list->next) {
+ for (i = 0; i < n_bits && i < list->n_bits; ++i) {
+ if ((key[i / 8] & (1 << (7 - i % 8))) !=
+ (list->key[i / 8] & (1 << (7 - i % 8))))
+ break;
+ }
+
+ if (i >= list->n_bits) {
+ if (!best || i > best->n_bits)
+ best = list;
+ }
+ }
+
+ return best;
+}
+
+static void test_lpm_basic(void)
+{
+ struct tlpm_node *list = NULL, *t1, *t2;
+
+ /* very basic, static tests to verify tlpm works as expected */
+
+ assert(!tlpm_match(list, (uint8_t[]){ 0xff }, 8));
+
+ t1 = list = tlpm_add(list, (uint8_t[]){ 0xff }, 8);
+ assert(t1 == tlpm_match(list, (uint8_t[]){ 0xff }, 8));
+ assert(t1 == tlpm_match(list, (uint8_t[]){ 0xff, 0xff }, 16));
+ assert(t1 == tlpm_match(list, (uint8_t[]){ 0xff, 0x00 }, 16));
+ assert(!tlpm_match(list, (uint8_t[]){ 0x7f }, 8));
+ assert(!tlpm_match(list, (uint8_t[]){ 0xfe }, 8));
+ assert(!tlpm_match(list, (uint8_t[]){ 0xff }, 7));
+
+ t2 = list = tlpm_add(list, (uint8_t[]){ 0xff, 0xff }, 16);
+ assert(t1 == tlpm_match(list, (uint8_t[]){ 0xff }, 8));
+ assert(t2 == tlpm_match(list, (uint8_t[]){ 0xff, 0xff }, 16));
+ assert(t1 == tlpm_match(list, (uint8_t[]){ 0xff, 0xff }, 15));
+ assert(!tlpm_match(list, (uint8_t[]){ 0x7f, 0xff }, 16));
+
+ tlpm_clear(list);
+}
+
+static void test_lpm_order(void)
+{
+ struct tlpm_node *t1, *t2, *l1 = NULL, *l2 = NULL;
+ size_t i, j;
+
+ /*
+ * Verify the tlpm implementation works correctly regardless of the
+ * order of entries. Insert a random set of entries into @l1, and copy
+ * the same data in reverse order into @l2. Then verify a lookup of
+ * random keys will yield the same result in both sets.
+ */
+
+ for (i = 0; i < (1 << 12); ++i)
+ l1 = tlpm_add(l1, (uint8_t[]){
+ rand() % 0xff,
+ rand() % 0xff,
+ }, rand() % 16 + 1);
+
+ for (t1 = l1; t1; t1 = t1->next)
+ l2 = tlpm_add(l2, t1->key, t1->n_bits);
+
+ for (i = 0; i < (1 << 8); ++i) {
+ uint8_t key[] = { rand() % 0xff, rand() % 0xff };
+
+ t1 = tlpm_match(l1, key, 16);
+ t2 = tlpm_match(l2, key, 16);
+
+ assert(!t1 == !t2);
+ if (t1) {
+ assert(t1->n_bits == t2->n_bits);
+ for (j = 0; j < t1->n_bits; ++j)
+ assert((t1->key[j / 8] & (1 << (7 - j % 8))) ==
+ (t2->key[j / 8] & (1 << (7 - j % 8))));
+ }
+ }
+
+ tlpm_clear(l1);
+ tlpm_clear(l2);
+}
+
+static void test_lpm_map(void)
+{
+ size_t i, j, n_matches, n_nodes, n_lookups;
+ struct tlpm_node *t, *list = NULL;
+ struct bpf_lpm_trie_key *key;
+ uint8_t value[8] = {};
+ int r, map;
+
+ /*
+ * Compare behavior of tlpm vs. bpf-lpm. Create a randomized set of
+ * prefixes and insert it into both tlpm and bpf-lpm. Then run some
+ * randomized lookups and verify both maps return the same result.
+ */
+
+ n_matches = 0;
+ n_nodes = 1 << 8;
+ n_lookups = 1 << 16;
+
+ key = alloca(sizeof(*key) + 4);
+ memset(key, 0, sizeof(*key) + 4);
+
+ map = bpf_map_create(BPF_MAP_TYPE_LPM_TRIE,
+ sizeof(*key) + 4,
+ sizeof(value),
+ 4096,
+ BPF_F_NO_PREALLOC);
+ assert(map >= 0);
+
+ for (i = 0; i < n_nodes; ++i) {
+ value[0] = rand() & 0xff;
+ value[1] = rand() & 0xff;
+ value[2] = rand() & 0xff;
+ value[3] = rand() & 0xff;
+ value[4] = rand() % 33;
+
+ list = tlpm_add(list, value, value[4]);
+
+ key->prefixlen = value[4];
+ memcpy(key->data, value, 4);
+ r = bpf_map_update(map, key, value, 0);
+ assert(!r);
+ }
+
+ for (i = 0; i < n_lookups; ++i) {
+ uint8_t data[] = {
+ rand() % 0xff,
+ rand() % 0xff,
+ rand() % 0xff,
+ rand() % 0xff
+ };
+
+ t = tlpm_match(list, data, 32);
+
+ key->prefixlen = 32;
+ memcpy(key->data, data, 4);
+ r = bpf_map_lookup(map, key, value);
+ assert(!r || errno == ENOENT);
+ assert(!t == !!r);
+
+ if (t) {
+ ++n_matches;
+ assert(t->n_bits == value[4]);
+ for (j = 0; j < t->n_bits; ++j)
+ assert((t->key[j / 8] & (1 << (7 - j % 8))) ==
+ (value[j / 8] & (1 << (7 - j % 8))));
+ }
+ }
+
+ close(map);
+ tlpm_clear(list);
+
+ /*
+ * With 255 random nodes in the map, we are pretty likely to match
+ * something on every lookup. For statistics, use this:
+ *
+ * printf(" nodes: %zu\n"
+ * "lookups: %zu\n"
+ * "matches: %zu\n", n_nodes, n_lookups, n_matches);
+ */
+}
+
+/* Test the implementation with some 'real world' examples */
+
+static void test_lpm_ipaddr(void)
+{
+ struct bpf_lpm_trie_key *key_ipv4;
+ struct bpf_lpm_trie_key *key_ipv6;
+ size_t key_size_ipv4;
+ size_t key_size_ipv6;
+ int map_fd_ipv4;
+ int map_fd_ipv6;
+ __u64 value;
+
+ key_size_ipv4 = sizeof(*key_ipv4) + sizeof(__u32);
+ key_size_ipv6 = sizeof(*key_ipv6) + sizeof(__u32) * 4;
+ key_ipv4 = alloca(key_size_ipv4);
+ key_ipv6 = alloca(key_size_ipv6);
+
+ map_fd_ipv4 = bpf_map_create(BPF_MAP_TYPE_LPM_TRIE,
+ key_size_ipv4, sizeof(value),
+ 100, BPF_F_NO_PREALLOC);
+ assert(map_fd_ipv4 >= 0);
+
+ map_fd_ipv6 = bpf_map_create(BPF_MAP_TYPE_LPM_TRIE,
+ key_size_ipv6, sizeof(value),
+ 100, BPF_F_NO_PREALLOC);
+ assert(map_fd_ipv6 >= 0);
+
+ /* Fill data some IPv4 and IPv6 address ranges */
+ value = 1;
+ key_ipv4->prefixlen = 16;
+ inet_pton(AF_INET, "192.168.0.0", key_ipv4->data);
+ assert(bpf_map_update(map_fd_ipv4, key_ipv4, &value, 0) == 0);
+
+ value = 2;
+ key_ipv4->prefixlen = 24;
+ inet_pton(AF_INET, "192.168.0.0", key_ipv4->data);
+ assert(bpf_map_update(map_fd_ipv4, key_ipv4, &value, 0) == 0);
+
+ value = 3;
+ key_ipv4->prefixlen = 24;
+ inet_pton(AF_INET, "192.168.128.0", key_ipv4->data);
+ assert(bpf_map_update(map_fd_ipv4, key_ipv4, &value, 0) == 0);
+
+ value = 5;
+ key_ipv4->prefixlen = 24;
+ inet_pton(AF_INET, "192.168.1.0", key_ipv4->data);
+ assert(bpf_map_update(map_fd_ipv4, key_ipv4, &value, 0) == 0);
+
+ value = 4;
+ key_ipv4->prefixlen = 23;
+ inet_pton(AF_INET, "192.168.0.0", key_ipv4->data);
+ assert(bpf_map_update(map_fd_ipv4, key_ipv4, &value, 0) == 0);
+
+ value = 0xdeadbeef;
+ key_ipv6->prefixlen = 64;
+ inet_pton(AF_INET6, "2a00:1450:4001:814::200e", key_ipv6->data);
+ assert(bpf_map_update(map_fd_ipv6, key_ipv6, &value, 0) == 0);
+
+ /* Set tprefixlen to maximum for lookups */
+ key_ipv4->prefixlen = 32;
+ key_ipv6->prefixlen = 128;
+
+ /* Test some lookups that should come back with a value */
+ inet_pton(AF_INET, "192.168.128.23", key_ipv4->data);
+ assert(bpf_map_lookup(map_fd_ipv4, key_ipv4, &value) == 0);
+ assert(value == 3);
+
+ inet_pton(AF_INET, "192.168.0.1", key_ipv4->data);
+ assert(bpf_map_lookup(map_fd_ipv4, key_ipv4, &value) == 0);
+ assert(value == 2);
+
+ inet_pton(AF_INET6, "2a00:1450:4001:814::", key_ipv6->data);
+ assert(bpf_map_lookup(map_fd_ipv6, key_ipv6, &value) == 0);
+ assert(value == 0xdeadbeef);
+
+ inet_pton(AF_INET6, "2a00:1450:4001:814::1", key_ipv6->data);
+ assert(bpf_map_lookup(map_fd_ipv6, key_ipv6, &value) == 0);
+ assert(value == 0xdeadbeef);
+
+ /* Test some lookups that should not match any entry */
+ inet_pton(AF_INET, "10.0.0.1", key_ipv4->data);
+ assert(bpf_map_lookup(map_fd_ipv4, key_ipv4, &value) == -1 &&
+ errno == ENOENT);
+
+ inet_pton(AF_INET, "11.11.11.11", key_ipv4->data);
+ assert(bpf_map_lookup(map_fd_ipv4, key_ipv4, &value) == -1 &&
+ errno == ENOENT);
+
+ inet_pton(AF_INET6, "2a00:ffff::", key_ipv6->data);
+ assert(bpf_map_lookup(map_fd_ipv6, key_ipv6, &value) == -1 &&
+ errno == ENOENT);
+
+ close(map_fd_ipv4);
+ close(map_fd_ipv6);
+}
+
+int main(void)
+{
+ struct rlimit limit = { RLIM_INFINITY, RLIM_INFINITY };
+ int ret;
+
+ /* we want predictable, pseudo random tests */
+ srand(0xf00ba1);
+
+ /* allow unlimited locked memory */
+ ret = setrlimit(RLIMIT_MEMLOCK, &limit);
+ if (ret < 0)
+ perror("Unable to lift memlock rlimit");
+
+ test_lpm_basic();
+ test_lpm_order();
+ test_lpm_map();
+ test_lpm_ipaddr();
+
+ printf("test_lpm: OK\n");
+ return 0;
+}
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] cxgb4: Remove redundant memset before memcpy
From: Tobias Klauser @ 2017-01-14 12:27 UTC (permalink / raw)
To: Shyam Saini; +Cc: hariprasad, netdev, linux-kernel
In-Reply-To: <1484356660-19082-1-git-send-email-mayhs11saini@gmail.com>
On 2017-01-14 at 02:17:40 +0100, Shyam Saini <mayhs11saini@gmail.com> wrote:
> The region set by the call to memset, immediately overwritten by
> the subsequent call to memcpy and thus makes the memset redundant.
>
> Also remove the memset((&info, 0, sizeof(info)) on line 398 because
> info is memcpy()'ed to before being used in the loop and it isn't
> used outside of the loop.
>
> Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
Reviewed-by: Tobias Klauser <tklauser@distanz.ch>
^ permalink raw reply
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jamal Hadi Salim @ 2017-01-14 12:56 UTC (permalink / raw)
To: John Fastabend, Jiri Pirko
Cc: Paul Blakey, David S. Miller, netdev, Jiri Pirko, Hadar Hen Zion,
Or Gerlitz, Roi Dayan, Roman Mashak, Simon Horman
In-Reply-To: <5873D523.4030301@gmail.com>
On 17-01-09 01:23 PM, John Fastabend wrote:
> On 17-01-08 09:19 AM, Jiri Pirko wrote:
[..]
>> This should never be interpreted by kernel. I think this would be good
>> to make clear in the comment in the code.
>>
>
> Ah OK I had assumed you would be pushing this via tc_cls_flower_offload into
> the driver in a follow up patch. But if it lives in kernel space as opaque
> cookie guess its no different then other id fields order/prio/cookie.
>
> Thanks for clarifying.
I think the feature makes a lot of sense (as is the action variant).
But can we make it:
a) fixed size
b) apply to all classifiers
c) please post a usage example via iproute2/tc
I am going to post the action variant in the next while - will do some
more testing first.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jamal Hadi Salim @ 2017-01-14 13:06 UTC (permalink / raw)
To: Paul Blakey, John Fastabend, David S. Miller, netdev
Cc: Jiri Pirko, Hadar Hen Zion, Or Gerlitz, Roi Dayan, Roman Mashak,
Simon Horman
In-Reply-To: <606f64cb-1744-d44f-8fd1-a4bdb1ca872f@mellanox.com>
On 17-01-03 07:22 AM, Paul Blakey wrote:
>
> I don't mind having it on TC level but I didn't want to intervene with
> all filters/TC.
>
Please do make it for all classifiers. I described a use case for u32
where the cookie could be used to pretty print the output on a
dump/get.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb
From: Oliver Hartkopp @ 2017-01-14 13:53 UTC (permalink / raw)
To: Liu Shuo, Eric Dumazet
Cc: linux-kernel, yanmin_zhang, shuox.liu, Zhang Yanmin, He, Bo,
Marc Kleine-Budde, David S. Miller, open list:CAN NETWORK LAYER,
open list:NETWORKING [GENERAL]
In-Reply-To: <20170114034330.GA30070@shuo-desktop.sh.intel.com>
Hello Eric,
On 01/14/2017 04:43 AM, Liu Shuo wrote:
> On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:
>> On 01/12/2017 02:01 PM, Eric Dumazet wrote:
>>> The main problem seems that the sockets themselves are not RCU
>>> protected.
>>>
>>> If CAN uses RCU for delivery, then sockets should be freed only after
>>> one RCU grace period.
>>>
>>> On recent kernels, following patch could help :
>>>
>>
>> Thanks Eric!
>>
>> @Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your
>> setup?
> Sorry for late reply. I was OOO yesterday.
> With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
> socket flag" in the latest kernel. With backporting this one plus Eric's
> following patch, it fixs my failure.
what would be the best approach to fix this issue - even in stable kernels?
E.g. would this change be ok for a stable as a quick fix?
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1108079d934f..6b974c2b66ef 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -112,6 +112,7 @@ EXPORT_SYMBOL(can_ioctl);
static void can_sock_destruct(struct sock *sk)
{
+ synchronize_rcu();
skb_queue_purge(&sk->sk_receive_queue);
}
And once this arrived in the mainline tree your suggested patch could be
applied?
In any case we should not forget to give Reported-by credits to Liu.
Best regards,
Oliver
^ permalink raw reply related
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jiri Pirko @ 2017-01-14 14:48 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: John Fastabend, Paul Blakey, David S. Miller, netdev, Jiri Pirko,
Hadar Hen Zion, Or Gerlitz, Roi Dayan, Roman Mashak, Simon Horman
In-Reply-To: <68e41fe4-5c4c-e159-fbfc-17d06638707d@mojatatu.com>
Sat, Jan 14, 2017 at 01:56:35PM CET, jhs@mojatatu.com wrote:
>On 17-01-09 01:23 PM, John Fastabend wrote:
>> On 17-01-08 09:19 AM, Jiri Pirko wrote:
>
>[..]
>> > This should never be interpreted by kernel. I think this would be good
>> > to make clear in the comment in the code.
>> >
>>
>> Ah OK I had assumed you would be pushing this via tc_cls_flower_offload into
>> the driver in a follow up patch. But if it lives in kernel space as opaque
>> cookie guess its no different then other id fields order/prio/cookie.
>>
>> Thanks for clarifying.
>
>
>I think the feature makes a lot of sense (as is the action variant).
>But can we make it:
>a) fixed size
Can you elaborate on why is this needed?
>b) apply to all classifiers
>c) please post a usage example via iproute2/tc
>
>I am going to post the action variant in the next while - will do some more
>testing first.
I believe we have to make the cls and ats cookies exactly the same.
>
>cheers,
>jamal
^ permalink raw reply
* [PATCH net-next 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-14 14:59 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim,
Jamal Hadi Salim
From: Jamal Hadi Salim <hadi@mojatatu.com>
Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it. The user can store whatever they wish in the
128 bits.
Sample exercise(using two 64bit values to represent the 128 bits):
.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
.. dump all gact actions..
sudo $TC -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 1221 sec used 27 sec
Action statistics:
Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
... show some stats
$ sudo $TC -s actions get action gact index 1
action order 1: gact action pass
random type none pass val 0
index 1 ref 3 bind 1 installed 2182 sec used 1 sec
Action statistics:
Sent 700344 bytes 9486 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/net/act_api.h | 7 +++++++
include/uapi/linux/pkt_cls.h | 7 +++++++
net/sched/act_api.c | 27 +++++++++++++++++++++++++--
3 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..b948db9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -20,6 +20,12 @@ struct tcf_hashinfo {
struct tc_action_ops;
+union act_cookie {
+ u16 ck16[8];
+ u32 ck32[4];
+ u64 ck64[2];
+};
+
struct tc_action {
const struct tc_action_ops *ops;
__u32 type; /* for backward compat(TCA_OLD_COMPAT) */
@@ -41,6 +47,7 @@ struct tc_action {
struct rcu_head tcfa_rcu;
struct gnet_stats_basic_cpu __percpu *cpu_bstats;
struct gnet_stats_queue __percpu *cpu_qstats;
+ union act_cookie *ck;
};
#define tcf_head common.tcfa_head
#define tcf_index common.tcfa_index
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1e5e1dd..6379af3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,12 @@
#include <linux/types.h>
#include <linux/pkt_sched.h>
+union u_act_cookie {
+ __u16 ck16[8];
+ __u32 ck32[4];
+ __u64 ck64[2];
+};
+
/* Action attributes */
enum {
TCA_ACT_UNSPEC,
@@ -12,6 +18,7 @@ enum {
TCA_ACT_INDEX,
TCA_ACT_STATS,
TCA_ACT_PAD,
+ TCA_ACT_COOKIE,
__TCA_ACT_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f04715a..85e77181 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
free_percpu(p->cpu_bstats);
free_percpu(p->cpu_qstats);
+ kfree(p->ck);
kfree(p);
}
@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
return a->ops->dump(skb, a, bind, ref);
}
-int
-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
+ int ref)
{
int err = -EINVAL;
unsigned char *b = skb_tail_pointer(skb);
@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
goto nla_put_failure;
if (tcf_action_copy_stats(skb, a, 0))
goto nla_put_failure;
+ if (a->ck) {
+ if (nla_put(skb, TCA_ACT_COOKIE, sizeof(union act_cookie),
+ a->ck))
+ goto nla_put_failure;
+ }
+
nest = nla_nest_start(skb, TCA_OPTIONS);
if (nest == NULL)
goto nla_put_failure;
@@ -575,6 +582,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
+ if (tb[TCA_ACT_COOKIE]) {
+ if (nla_len(tb[TCA_ACT_COOKIE]) != sizeof(union act_cookie)) {
+ err = -EINVAL;
+ goto err_mod;
+ }
+
+ a->ck = kzalloc(sizeof(union act_cookie), GFP_KERNEL);
+ if (unlikely(!a->ck)) {
+ err = -ENOMEM;
+ goto err_mod;
+ }
+
+ memcpy((void *)a->ck, nla_data(tb[TCA_ACT_COOKIE]),
+ sizeof(union act_cookie));
+ }
+
/* module count goes up only when brand new policy is created
* if it exists and is only bound to in a_o->init() then
* ACT_P_CREATED is not returned (a zero is).
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jamal Hadi Salim @ 2017-01-14 15:03 UTC (permalink / raw)
To: Jiri Pirko
Cc: John Fastabend, Paul Blakey, David S. Miller, netdev, Jiri Pirko,
Hadar Hen Zion, Or Gerlitz, Roi Dayan, Roman Mashak, Simon Horman
In-Reply-To: <20170114144855.GA1886@nanopsycho.orion>
On 17-01-14 09:48 AM, Jiri Pirko wrote:
> Sat, Jan 14, 2017 at 01:56:35PM CET, jhs@mojatatu.com wrote:
>> I think the feature makes a lot of sense (as is the action variant).
>> But can we make it:
>> a) fixed size
>
> Can you elaborate on why is this needed?
>
My experience with the action bits its easier to debug
and enforces some discipline to not abuse the amount of RAM used.
If you have 1M rules, one extra 128M is easier on the system than
a few Gigs.
>
>> b) apply to all classifiers
>> c) please post a usage example via iproute2/tc
>>
>> I am going to post the action variant in the next while - will do some more
>> testing first.
>
> I believe we have to make the cls and ats cookies exactly the same.
>
Probably - they are both needed. See the patch I just posted.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies
From: Jiri Pirko @ 2017-01-14 15:22 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: davem, netdev, jiri, paulb, john.fastabend, simon.horman, mrv,
hadarh, ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
In-Reply-To: <1484405958-24591-1-git-send-email-jhs@emojatatu.com>
Sat, Jan 14, 2017 at 03:59:18PM CET, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <hadi@mojatatu.com>
>
>Introduce optional 128-bit action cookie.
>Like all other cookie schemes in the networking world (eg in protocols
>like http or existing kernel fib protocol field, etc) the idea is to save
>user state that when retrieved serves as a correlator. The kernel
>_should not_ intepret it. The user can store whatever they wish in the
>128 bits.
>
>Sample exercise(using two 64bit values to represent the 128 bits):
>
>.. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
2x 64bit values? Why can't this have variable length, according to what
user needs:
sudo $TC actions add action ok index 1 cookie a0
sudo $TC actions add action ok index 1 cookie a01122
sudo $TC actions add action ok index 1 cookie a01122334455
sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>
>.. dump all gact actions..
>sudo $TC -s actions ls action gact
>
> action order 0: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 1221 sec used 27 sec
> Action statistics:
> Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
Input is 2x64 and dump is 4x32? That is confusing. With my suggested
example, this would be:
cookie a0
cookie a01122
cookie a01122334455
cookie a01122334455aabbccddeeff
>
>.. bind the accept action to a filter..
>sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
>u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
>... send some traffic..
>$ ping 127.0.0.1 -c 3
>PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
>64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
>64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
>64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
>--- 127.0.0.1 ping statistics ---
>3 packets transmitted, 3 received, 0% packet loss, time 2109ms
>rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
>... show some stats
>$ sudo $TC -s actions get action gact index 1
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 3 bind 1 installed 2182 sec used 1 sec
> Action statistics:
> Sent 700344 bytes 9486 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/net/act_api.h | 7 +++++++
> include/uapi/linux/pkt_cls.h | 7 +++++++
> net/sched/act_api.c | 27 +++++++++++++++++++++++++--
> 3 files changed, 39 insertions(+), 2 deletions(-)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index 1d71644..b948db9 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -20,6 +20,12 @@ struct tcf_hashinfo {
>
> struct tc_action_ops;
>
>+union act_cookie {
>+ u16 ck16[8];
>+ u32 ck32[4];
>+ u64 ck64[2];
Since this should be never interpreted by kernel, I don't understand why
this union is needed. Why just don't pass a char array?
Also, whatever format this is, could we make is shared with cls cookie?
>+};
>+
> struct tc_action {
> const struct tc_action_ops *ops;
> __u32 type; /* for backward compat(TCA_OLD_COMPAT) */
>@@ -41,6 +47,7 @@ struct tc_action {
> struct rcu_head tcfa_rcu;
> struct gnet_stats_basic_cpu __percpu *cpu_bstats;
> struct gnet_stats_queue __percpu *cpu_qstats;
>+ union act_cookie *ck;
> };
> #define tcf_head common.tcfa_head
> #define tcf_index common.tcfa_index
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index 1e5e1dd..6379af3 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -4,6 +4,12 @@
> #include <linux/types.h>
> #include <linux/pkt_sched.h>
>
>+union u_act_cookie {
>+ __u16 ck16[8];
>+ __u32 ck32[4];
>+ __u64 ck64[2];
>+};
Again, the same struct? I don't understand why twice.
>+
> /* Action attributes */
> enum {
> TCA_ACT_UNSPEC,
>@@ -12,6 +18,7 @@ enum {
> TCA_ACT_INDEX,
> TCA_ACT_STATS,
> TCA_ACT_PAD,
>+ TCA_ACT_COOKIE,
> __TCA_ACT_MAX
> };
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index f04715a..85e77181 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -33,6 +33,7 @@ static void free_tcf(struct rcu_head *head)
>
> free_percpu(p->cpu_bstats);
> free_percpu(p->cpu_qstats);
>+ kfree(p->ck);
> kfree(p);
> }
>
>@@ -464,8 +465,8 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> return a->ops->dump(skb, a, bind, ref);
> }
>
>-int
>-tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>+int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind,
>+ int ref)
> {
> int err = -EINVAL;
> unsigned char *b = skb_tail_pointer(skb);
>@@ -475,6 +476,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
> goto nla_put_failure;
> if (tcf_action_copy_stats(skb, a, 0))
> goto nla_put_failure;
>+ if (a->ck) {
>+ if (nla_put(skb, TCA_ACT_COOKIE, sizeof(union act_cookie),
>+ a->ck))
>+ goto nla_put_failure;
>+ }
>+
> nest = nla_nest_start(skb, TCA_OPTIONS);
> if (nest == NULL)
> goto nla_put_failure;
>@@ -575,6 +582,22 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> if (err < 0)
> goto err_mod;
>
>+ if (tb[TCA_ACT_COOKIE]) {
>+ if (nla_len(tb[TCA_ACT_COOKIE]) != sizeof(union act_cookie)) {
>+ err = -EINVAL;
>+ goto err_mod;
>+ }
>+
>+ a->ck = kzalloc(sizeof(union act_cookie), GFP_KERNEL);
>+ if (unlikely(!a->ck)) {
>+ err = -ENOMEM;
>+ goto err_mod;
>+ }
>+
>+ memcpy((void *)a->ck, nla_data(tb[TCA_ACT_COOKIE]),
>+ sizeof(union act_cookie));
>+ }
>+
> /* module count goes up only when brand new policy is created
> * if it exists and is only bound to in a_o->init() then
> * ACT_P_CREATED is not returned (a zero is).
>--
>1.9.1
>
^ permalink raw reply
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jiri Pirko @ 2017-01-14 15:29 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: John Fastabend, Paul Blakey, David S. Miller, netdev, Jiri Pirko,
Hadar Hen Zion, Or Gerlitz, Roi Dayan, Roman Mashak, Simon Horman
In-Reply-To: <de59c019-8a7d-6747-ddc1-9a21f40d3e04@mojatatu.com>
Sat, Jan 14, 2017 at 04:03:17PM CET, jhs@mojatatu.com wrote:
>On 17-01-14 09:48 AM, Jiri Pirko wrote:
>> Sat, Jan 14, 2017 at 01:56:35PM CET, jhs@mojatatu.com wrote:
>
>
>> > I think the feature makes a lot of sense (as is the action variant).
>> > But can we make it:
>> > a) fixed size
>>
>> Can you elaborate on why is this needed?
>>
>
>My experience with the action bits its easier to debug
>and enforces some discipline to not abuse the amount of RAM used.
>If you have 1M rules, one extra 128M is easier on the system than
>a few Gigs.
Fair. So could this be done like IFLA_PHYS_SWITCH_ID and
IFLA_PHYS_PORT_ID. They can have variable size, max is MAX_PHYS_ITEM_ID_LEN
We can let user to pass arbitrary len up to 16 bytes. This has benefit in
fact that if in future this needs to be extended to say 32 bytes, it is
backward compatible. We just change the check in kernel.
>
>>
>> > b) apply to all classifiers
>> > c) please post a usage example via iproute2/tc
>> >
>> > I am going to post the action variant in the next while - will do some more
>> > testing first.
>>
>> I believe we have to make the cls and ats cookies exactly the same.
>>
>
>Probably - they are both needed. See the patch I just posted.
>
>cheers,
>jamal
>
^ permalink raw reply
* Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-14 15:39 UTC (permalink / raw)
To: Jiri Pirko
Cc: davem, netdev, jiri, paulb, john.fastabend, simon.horman, mrv,
hadarh, ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
In-Reply-To: <20170114152248.GB1886@nanopsycho.orion>
On 17-01-14 10:22 AM, Jiri Pirko wrote:
>> .. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>> sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
>
> 2x 64bit values? Why can't this have variable length, according to what
> user needs:
You can intepret it however you wish. It is 128 bits. You can make it
2x64, 4x32, 8x16, 16x8
>
> sudo $TC actions add action ok index 1 cookie a0
> sudo $TC actions add action ok index 1 cookie a01122
> sudo $TC actions add action ok index 1 cookie a01122334455
> sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>
Sure you can do that too..
I will add add 16 8b fields to the union.
>>
>> .. dump all gact actions..
>> sudo $TC -s actions ls action gact
>>
>> action order 0: gact action pass
>> random type none pass val 0
>> index 1 ref 2 bind 1 installed 1221 sec used 27 sec
>> Action statistics:
>> Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>> cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
>
> Input is 2x64 and dump is 4x32? That is confusing. With my suggested
> example, this would be:
>
> cookie a0
> cookie a01122
> cookie a01122334455
> cookie a01122334455aabbccddeeff
>
Your suggestion is more sensible for a user space cli tool like tc.
I will add a uchar cku8[16] field and make changes to iproute2.
>> struct tc_action_ops;
>>
>> +union act_cookie {
>> + u16 ck16[8];
>> + u32 ck32[4];
>> + u64 ck64[2];
>
> Since this should be never interpreted by kernel, I don't understand why
> this union is needed. Why just don't pass a char array?
>
programmatic usability.
> Also, whatever format this is, could we make is shared with cls cookie?
>
>
The structure could be shared (and because it is in pkt_cls.h
that makes it easier). But the TLVs are domain specific. We need another
one for classifiers.
>> +};
>> +
>> struct tc_action {
>> const struct tc_action_ops *ops;
>> __u32 type; /* for backward compat(TCA_OLD_COMPAT) */
>> @@ -41,6 +47,7 @@ struct tc_action {
>> struct rcu_head tcfa_rcu;
>> struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> struct gnet_stats_queue __percpu *cpu_qstats;
>> + union act_cookie *ck;
>> };
>> #define tcf_head common.tcfa_head
>> #define tcf_index common.tcfa_index
>> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> index 1e5e1dd..6379af3 100644
>> --- a/include/uapi/linux/pkt_cls.h
>> +++ b/include/uapi/linux/pkt_cls.h
>> @@ -4,6 +4,12 @@
>> #include <linux/types.h>
>> #include <linux/pkt_sched.h>
>>
>> +union u_act_cookie {
>> + __u16 ck16[8];
>> + __u32 ck32[4];
>> + __u64 ck64[2];
>> +};
>
> Again, the same struct? I don't understand why twice.
Just old habits.
user vs kernel api? Standard action approach one says
__u32 other says u32; hanging off the user variant to kernel
didnt feel right.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 1/1] net sched actions: Add support for user cookies
From: Jiri Pirko @ 2017-01-14 15:49 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: davem, netdev, jiri, paulb, john.fastabend, simon.horman, mrv,
hadarh, ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
In-Reply-To: <09bf24f3-7080-341d-a332-a2042e7fc34b@mojatatu.com>
Sat, Jan 14, 2017 at 04:39:24PM CET, jhs@mojatatu.com wrote:
>On 17-01-14 10:22 AM, Jiri Pirko wrote:
>
>> > .. create an accept action with cookie 0xA:0xa0a0a0a0a0a0a0
>> > sudo $TC actions add action ok index 1 cookie 0xA 0xa0a0a0a0a0a0a0
>>
>> 2x 64bit values? Why can't this have variable length, according to what
>> user needs:
>
>
>You can intepret it however you wish. It is 128 bits. You can make it
>2x64, 4x32, 8x16, 16x8
>
>>
>> sudo $TC actions add action ok index 1 cookie a0
>> sudo $TC actions add action ok index 1 cookie a01122
>> sudo $TC actions add action ok index 1 cookie a01122334455
>> sudo $TC actions add action ok index 1 cookie a01122334455aabbccddeeff
>>
>
>Sure you can do that too..
>I will add add 16 8b fields to the union.
>
>
>> >
>> > .. dump all gact actions..
>> > sudo $TC -s actions ls action gact
>> >
>> > action order 0: gact action pass
>> > random type none pass val 0
>> > index 1 ref 2 bind 1 installed 1221 sec used 27 sec
>> > Action statistics:
>> > Sent 373248 bytes 5056 pkt (dropped 0, overlimits 0 requeues 0)
>> > backlog 0b 0p requeues 0
>> > cookie(0000000a:00000000:a0a0a0a0:00a0a0a0)
>>
>> Input is 2x64 and dump is 4x32? That is confusing. With my suggested
>> example, this would be:
>>
>> cookie a0
>> cookie a01122
>> cookie a01122334455
>> cookie a01122334455aabbccddeeff
>>
>
>Your suggestion is more sensible for a user space cli tool like tc.
>I will add a uchar cku8[16] field and make changes to iproute2.
>
>> > struct tc_action_ops;
>> >
>> > +union act_cookie {
>> > + u16 ck16[8];
>> > + u32 ck32[4];
>> > + u64 ck64[2];
>>
>> Since this should be never interpreted by kernel, I don't understand why
>> this union is needed. Why just don't pass a char array?
>>
>
>programmatic usability.
I don't see why. In userspace you can map whatever struct you need to the
mem with chararray. It's totally up to you as an app developer. There is
no need to make that part of kernel api. Really.
>
>> Also, whatever format this is, could we make is shared with cls cookie?
>>
>>
>
>The structure could be shared (and because it is in pkt_cls.h
>that makes it easier). But the TLVs are domain specific. We need another
>one for classifiers.
>
>> > +};
>> > +
>> > struct tc_action {
>> > const struct tc_action_ops *ops;
>> > __u32 type; /* for backward compat(TCA_OLD_COMPAT) */
>> > @@ -41,6 +47,7 @@ struct tc_action {
>> > struct rcu_head tcfa_rcu;
>> > struct gnet_stats_basic_cpu __percpu *cpu_bstats;
>> > struct gnet_stats_queue __percpu *cpu_qstats;
>> > + union act_cookie *ck;
>> > };
>> > #define tcf_head common.tcfa_head
>> > #define tcf_index common.tcfa_index
>> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>> > index 1e5e1dd..6379af3 100644
>> > --- a/include/uapi/linux/pkt_cls.h
>> > +++ b/include/uapi/linux/pkt_cls.h
>> > @@ -4,6 +4,12 @@
>> > #include <linux/types.h>
>> > #include <linux/pkt_sched.h>
>> >
>> > +union u_act_cookie {
>> > + __u16 ck16[8];
>> > + __u32 ck32[4];
>> > + __u64 ck64[2];
>> > +};
>>
>> Again, the same struct? I don't understand why twice.
>
>Just old habits.
>user vs kernel api? Standard action approach one says
>__u32 other says u32; hanging off the user variant to kernel
>didnt feel right.
Just have it in uapi and use it from within kernel. But did you see what
I suggested in the other thread (regarding IFLA_PHYS_PORT_ID and
IFLA_PHYS_SWITCH_ID)? If you do it what way, you don't need no struct.
^ permalink raw reply
* Re: [PATCH] net: add regs attribute to phy device for user diagnose
From: Andrew Lunn @ 2017-01-14 16:24 UTC (permalink / raw)
To: yuan linyu; +Cc: Florian Fainelli, David S . Miller, netdev, yuan linyu
In-Reply-To: <1484361992-30591-1-git-send-email-cugyly@163.com>
On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>
> if phy device have register(s) configuration problem,
> user can use this attribute to diagnose.
> this feature need phy driver maintainer implement.
what is wrong with mii-tool -vv ?
Andrew
^ permalink raw reply
* (unknown),
From: pooks005 @ 2017-01-14 16:34 UTC (permalink / raw)
To: netdev
[-- Attachment #1: EMAIL_25645_netdev.zip --]
[-- Type: application/zip, Size: 45410 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/2] bpf: add a longest prefix match trie map implementation
From: Alexei Starovoitov @ 2017-01-14 16:55 UTC (permalink / raw)
To: Daniel Mack; +Cc: dh.herrmann, daniel, netdev, davem
In-Reply-To: <20170114121727.14784-2-daniel@zonque.org>
On 1/14/17 4:17 AM, Daniel Mack wrote:
> +static struct bpf_map *trie_alloc(union bpf_attr *attr)
> +{
> + size_t cost, cost_per_node;
> + struct lpm_trie *trie;
> + int ret;
> +
> + /* check sanity of attributes */
> + if (attr->max_entries == 0 ||
> + attr->map_flags != BPF_F_NO_PREALLOC ||
> + attr->key_size < sizeof(struct bpf_lpm_trie_key) + 1 ||
> + attr->key_size > sizeof(struct bpf_lpm_trie_key) + 256 ||
> + attr->value_size == 0)
> + return ERR_PTR(-EINVAL);
could you also make it root only for now ?
Like we did for lru map:
if (lru && !capable(CAP_SYS_ADMIN))
/* LRU implementation is much complicated than other
* maps. Hence, limit to CAP_SYS_ADMIN for now.
*/
return ERR_PTR(-EPERM);
trie is not that complicated, but I think socket_filters
(the only unpriv prog type today) can live a release or
two without ability to use it.
In patch 2/2 there are some comments not in networking style:
+ /*
+ * Perform longest prefix-match on @key/@n_bits. That is,
As far as performance it has to be measured from datapath.
Like create tc+cls_bpf prog that does a dozen of trie lookups,
attach it to veth or tap and use
samples/pktgen/pktgen_bench_xmit_mode_netif_receive.sh
to send traffic into it.
Like: samples/bpf/test_cls_bpf.sh does.
Another alternative is to extend samples/bpf/map_perf_test
It has perf tests for most map types today (including lru)
and trie would be natural addition there.
I would prefer this latter option.
Thanks!
^ permalink raw reply
* Re: [PATCH] cxgb4: Remove redundant memset before memcpy
From: David Miller @ 2017-01-14 17:00 UTC (permalink / raw)
To: mayhs11saini; +Cc: hariprasad, tklauser, netdev, linux-kernel
In-Reply-To: <1484356660-19082-1-git-send-email-mayhs11saini@gmail.com>
From: Shyam Saini <mayhs11saini@gmail.com>
Date: Sat, 14 Jan 2017 06:47:40 +0530
> The region set by the call to memset, immediately overwritten by
> the subsequent call to memcpy and thus makes the memset redundant.
>
> Also remove the memset((&info, 0, sizeof(info)) on line 398 because
> info is memcpy()'ed to before being used in the loop and it isn't
> used outside of the loop.
>
> Signed-off-by: Shyam Saini <mayhs11saini@gmail.com>
Applied to net-next, thanks.
^ permalink raw reply
* Re: pull-request: mac80211-next 2017-01-13
From: David Miller @ 2017-01-14 17:04 UTC (permalink / raw)
To: johannes-cdvu00un1VgdHxzADdlk8Q
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170113102933.8246-1-johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Fri, 13 Jan 2017 11:29:32 +0100
> This is my first pull request for net-next, and it seems a bit
> bigger than the past few releases. Detailed information below,
> as usual.
>
> Please pull and let me know if there's any problem.
Pulled, thanks Johannes.
^ permalink raw reply
* Re: [PATCH] can: Fix kernel panic at security_sock_rcv_skb
From: Eric Dumazet @ 2017-01-14 17:30 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: Liu Shuo, linux-kernel, yanmin_zhang, shuox.liu, Zhang Yanmin,
He, Bo, Marc Kleine-Budde, David S. Miller,
open list:CAN NETWORK LAYER, open list:NETWORKING [GENERAL]
In-Reply-To: <6dc1d111-9aa4-5771-0283-6408567e47c2@hartkopp.net>
On Sat, 2017-01-14 at 14:53 +0100, Oliver Hartkopp wrote:
> Hello Eric,
>
> On 01/14/2017 04:43 AM, Liu Shuo wrote:
> > On Thu 12.Jan'17 at 17:33:38 +0100, Oliver Hartkopp wrote:
> >> On 01/12/2017 02:01 PM, Eric Dumazet wrote:
>
> >>> The main problem seems that the sockets themselves are not RCU
> >>> protected.
> >>>
> >>> If CAN uses RCU for delivery, then sockets should be freed only after
> >>> one RCU grace period.
> >>>
> >>> On recent kernels, following patch could help :
> >>>
> >>
> >> Thanks Eric!
> >>
> >> @Liu ShuoX: Can you check if Eric's suggestion fixes the issue in your
> >> setup?
> > Sorry for late reply. I was OOO yesterday.
> > With Eric's hint, i just found his patch that "net: add SOCK_RCU_FREE
> > socket flag" in the latest kernel. With backporting this one plus Eric's
> > following patch, it fixs my failure.
>
> what would be the best approach to fix this issue - even in stable kernels?
>
> E.g. would this change be ok for a stable as a quick fix?
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 1108079d934f..6b974c2b66ef 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -112,6 +112,7 @@ EXPORT_SYMBOL(can_ioctl);
>
> static void can_sock_destruct(struct sock *sk)
> {
> + synchronize_rcu();
> skb_queue_purge(&sk->sk_receive_queue);
> }
Adding a synchronize_rcu() at socket close time might have side effects,
if say an application had 1000 such sockets and dies.
This might add 20 seconds of exit time and have serious implications.
I will submit the second patch : It is working for all linux versions.
>
> And once this arrived in the mainline tree your suggested patch could be
> applied?
>
> In any case we should not forget to give Reported-by credits to Liu.
Sure
^ permalink raw reply
* Re: [pull request][for-next] Mellanox mlx5 Reorganize core driver directory layout
From: Tom Herbert @ 2017-01-14 17:37 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Leon Romanovsky, David Miller, Saeed Mahameed, Doug Ledford,
Linux Kernel Network Developers,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CALzJLG_RsvgszVxGZ7+VgYjET9w=XTjq36Z72diOQFNJWz9tQw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Jan 13, 2017 at 2:45 PM, Saeed Mahameed
<saeedm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On Sat, Jan 14, 2017 at 12:06 AM, Tom Herbert <tom-BjP2VixgY4xUbtYUoyoikg@public.gmane.org> wrote:
>> On Fri, Jan 13, 2017 at 12:29 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> On Fri, Jan 13, 2017 at 12:14:07PM -0500, David Miller wrote:
>>>> From: Saeed Mahameed <saeedm-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> Date: Thu, 12 Jan 2017 19:22:34 +0200
>>>>
>>>> > This pull request includes one patch from Leon, this patch as described
>>>> > below will change the driver directory structure and layout for better,
>>>> > logical and modular driver files separation.
>>>> >
>>>> > This change is important to both rdma and net maintainers in order to
>>>> > have smoother management of driver patches for different mlx5 sub modules
>>>> > and smoother rdma-next vs. net-next features submissions.
>>>> >
>>>> > Please find more info below -in the tag commit message-,
>>>> > review and let us know if there's any problem.
>>>> >
>>>> > This change doesn't introduce any conflicts with the current mlx5
>>>> > fixes and cleanups posted on 2017-01-10 to net branch, and merge tests
>>>> > worked flawlessly with no issues.
>>>> >
>>>> > This is the last pull request meant for both rdma-next and net-next.
>>>> > Once pulled, this will be the base shared code for both trees.
>>>>
>>>> This is pretty crazy, it will make all bug fix backporting to -stable
>>>> a complete nightmare for myself, Doug, various distribution maintainers
>>>> and many other people who quietly have to maintain their own trees and
>>>> do backporting.
>>>
>>> Hi Dave,
>>>
>>> I understand your worries, but our case is similar to various other
>>> drivers, for example hfi1 which was in staging for years while
>>> supported in RedHat and moved from there to IB. The Chelsio drivers did
>>> similar reorg in 2016 (drivers/net/ethernet/chelsio/libcxgb) while their
>>> drivers were in the tree for long time before.
>>>
>>> Additionally, Doug doesn't need to maintain -stable queue and it is done
>>> by relevant submaintainers who are adding stable tags by themselves. In
>>> the IB case, the burden will continue to be on me and not on Doug.
>>>
>> Recently I had to backport the mlx5 driver from 4.9 to 4.6 in order to
>> get support for XDP. The biggest issue I faced was the lack of
>> modularity in the many driver features that are now supported. The
>> problem with backporting these new features is the spider web of
>> dependencies that they bring in from the rest of the kernel. I ended
>> up taking out en_rep, vxlan, en_tc, eswitch, and dcbnl. The result was
>> ~340 patches which is still a lot but at least this was constrained to
>> patches in the mlx5 directories and are relevant to what we want to
>> do.
>>
>> In lieu of restructuring the directories, I would much rather see more
>> config options so that we can build drivers that don't unnecessarily
>> complicate our lives with features we don't use. This is not just true
>> for Mellanox, but I would say it would be true of any driver that
>> someone is trying to deploy and maintain at large scale.
>>
>
> I think we should have both, if the restructuring made right,
> new whole features (e.g eswitch and eswitch offlaods or any independent module),
> can sit in their own directory and keep their own logic concentrated
> in one place, and only touch the
> main driver code with simple entry points in the main flow, this way
> you can simply compile their whole directories
> out with a config flag directly from the Makefile.
>
That would be nice, but that is not what your new layout gives us yet.
The starting point for this structure should be to discern what the
minimum set of files is to build a functional and good performance
driver with the basic functionality including only the most common
offloads. These I would think constitute the core files for the driver
and hence make sense to be in the "core" directory. Personally I would
drop the en_ file prefix and not make an en directory, we're already
in the ethernet driver part of the tree so I don't know what this name
is supposed to convey. For reference I gave you this list of
components I disabled to do the backport, some we're not split out in
your directory layout. A good example is the VXLAN offload support,
the changes in that area we're mostly due to Alexander's patch set to
redo the interface for these accelerations-- so to back port driver
from 4.9 to 4.6 I would need to pull those in and whatever else those
depend on. It was way easier to just ifdef out the relevant calls and
not compile the vxlan files to do the backport.
Tom
>> Btw, we did hit one issue in the backport. We started to get rx csum
>> faults (checksum complete value indicates TCP checksum is bad, but
>> host computation says checksum is good). I ran against 4.9 upstream
>> kernel and do see these, however don't see them in 4.10. I haven't
>> bisected yet. Is this a known issue?
>>
>
> Not to me, I don't recall any csum related fixes or feature submitted
> lately to mlx5,
> Maybe something changed in the stack ?
>
> what configuration are you running ? what traffic ?
>
>> Thanks,
>> Tom
>>
>>>>
>>>> I really don't think you can justify this rearrangement based upon the
>>>> consequences and how much activity happens in this driver.
>>>>
>>>> You should have thought long and hard about the layout a long time ago
>>>> rather than after the driver has been in the tree for many years.
>>>>
>>>> Sorry.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] net/sched: cls_flower: Add user specified data
From: Jamal Hadi Salim @ 2017-01-14 17:46 UTC (permalink / raw)
To: Jiri Pirko
Cc: John Fastabend, Paul Blakey, David S. Miller, netdev, Jiri Pirko,
Hadar Hen Zion, Or Gerlitz, Roi Dayan, Roman Mashak, Simon Horman
In-Reply-To: <20170114152918.GC1886@nanopsycho.orion>
On 17-01-14 10:29 AM, Jiri Pirko wrote:
> Sat, Jan 14, 2017 at 04:03:17PM CET, jhs@mojatatu.com wrote:
> Fair. So could this be done like IFLA_PHYS_SWITCH_ID and
> IFLA_PHYS_PORT_ID. They can have variable size, max is MAX_PHYS_ITEM_ID_LEN
>
> We can let user to pass arbitrary len up to 16 bytes. This has benefit in
> fact that if in future this needs to be extended to say 32 bytes, it is
> backward compatible. We just change the check in kernel.
>
Sure. Ive run out of time for now; but will work on a new version later.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] net: korina: use new api ethtool_{get|set}_link_ksettings
From: Florian Fainelli @ 2017-01-14 18:34 UTC (permalink / raw)
To: Philippe Reynes, davem, mugunthanvnm, a, fw, jarod; +Cc: netdev, linux-kernel
In-Reply-To: <1484393599-1547-1-git-send-email-tremyfr@gmail.com>
On 01/14/2017 03:33 AM, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
>
> Signed-off-by: Philippe Reynes <tremyfr@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH] net: add regs attribute to phy device for user diagnose
From: Florian Fainelli @ 2017-01-14 18:35 UTC (permalink / raw)
To: Andrew Lunn, yuan linyu; +Cc: David S . Miller, netdev, yuan linyu
In-Reply-To: <20170114162453.GB1072@lunn.ch>
On 01/14/2017 08:24 AM, Andrew Lunn wrote:
> On Sat, Jan 14, 2017 at 10:46:31AM +0800, yuan linyu wrote:
>> From: yuan linyu <Linyu.Yuan@alcatel-sbell.com.cn>
>>
>> if phy device have register(s) configuration problem,
>> user can use this attribute to diagnose.
>> this feature need phy driver maintainer implement.
>
> what is wrong with mii-tool -vv ?
Agreed, and without an actual user of this API (ethtool?), nor a PHY
driver implementing it, we cannot quite see how you want to make use of
this.
Thank you
--
Florian
^ permalink raw reply
* Re: [PATCH v5 01/13] net: ethernet: aquantia: Make and configuration files.
From: Florian Fainelli @ 2017-01-14 18:39 UTC (permalink / raw)
To: Alexander Loktionov, netdev, David VomLehn
Cc: David S . Miller, Simon Edelhaus, Dmitrii Tarakanov, Pavel Belous,
Dmitry Bezrukov
In-Reply-To: <fe1664e502d28bcb6a5689787b8000e6c9288307.1484283610.git.vomlehn@texas.net>
On 01/12/2017 09:02 PM, Alexander Loktionov wrote:
> From: David VomLehn <vomlehn@texas.net>
>
> Patches to create the make and configuration files.
>
> Signed-off-by: Alexander Loktionov <Alexander.Loktionov@aquantia.com>
> Signed-off-by: Dmitrii Tarakanov <Dmitrii.Tarakanov@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
> Signed-off-by: Dmitry Bezrukov <Dmitry.Bezrukov@aquantia.com>
> Signed-off-by: David M. VomLehn <vomlehn@texas.net>
> ---
> drivers/net/ethernet/aquantia/Kconfig | 24 +++++++++++++++++++
> drivers/net/ethernet/aquantia/Makefile | 42 ++++++++++++++++++++++++++++++++++
> drivers/net/ethernet/aquantia/ver.h | 18 +++++++++++++++
> 3 files changed, 84 insertions(+)
> create mode 100644 drivers/net/ethernet/aquantia/Kconfig
> create mode 100644 drivers/net/ethernet/aquantia/Makefile
> create mode 100644 drivers/net/ethernet/aquantia/ver.h
>
> diff --git a/drivers/net/ethernet/aquantia/Kconfig b/drivers/net/ethernet/aquantia/Kconfig
> new file mode 100644
> index 0000000..a74a4c0
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Kconfig
> @@ -0,0 +1,24 @@
> +#
> +# aQuantia device configuration
> +#
> +
> +config NET_VENDOR_AQUANTIA
> + bool "aQuantia devices"
> + default y
> + ---help---
> + Set this to y if you have an Ethernet network cards that uses the aQuantia
> + chipset.
> +
> + This option does not build any drivers; it casues the aQuantia
> + drivers that can be built to appear in the list of Ethernet drivers.
> +
> +
> +if NET_VENDOR_AQUANTIA
> +
> +config AQTION
> + tristate "aQuantia AQtion Support"
> + depends on PCI
> + ---help---
> + This enables the support for the aQuantia AQtion Ethernet card.
> +
> +endif # NET_VENDOR_AQUANTIA
> diff --git a/drivers/net/ethernet/aquantia/Makefile b/drivers/net/ethernet/aquantia/Makefile
> new file mode 100644
> index 0000000..e4ae696
> --- /dev/null
> +++ b/drivers/net/ethernet/aquantia/Makefile
> @@ -0,0 +1,42 @@
> +################################################################################
> +#
> +# aQuantia Ethernet Controller AQtion Linux Driver
> +# Copyright(c) 2014-2017 aQuantia Corporation.
> +#
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms and conditions of the GNU General Public License,
> +# version 2, as published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along
> +# with this program. If not, see <http://www.gnu.org/licenses/>.
> +#
> +# The full GNU General Public License is included in this distribution in
> +# the file called "COPYING".
> +#
> +# Contact Information: <rdc-drv@aquantia.com>
> +# aQuantia Corporation, 105 E. Tasman Dr. San Jose, CA 95134, USA
> +#
> +################################################################################
> +
> +#
> +# Makefile for the AQtion(tm) Ethernet driver
> +#
> +
> +obj-$(CONFIG_AQTION) += atlantic.o
> +
> +atlantic-objs := aq_main.o \
> + aq_nic.o \
> + aq_pci_func.o \
> + aq_vec.o \
> + aq_ring.o \
> + aq_hw_utils.o \
> + aq_ethtool.o \
> + hw_atl/hw_atl_a0.o \
> + hw_atl/hw_atl_b0.o \
> + hw_atl/hw_atl_utils.o \
> + hw_atl/hw_atl_llh.o
You might want to create an aqtion or atlantic folder just in case you
later on submit a driver for another aquantia NIC. That would keep the
directory structure clean.
--
Florian
^ permalink raw reply
* Re: [PATCH net] openvswitch: maintain correct checksum state in conntrack actions
From: Pravin Shelar @ 2017-01-14 18:42 UTC (permalink / raw)
To: Lance Richardson; +Cc: Linux Kernel Network Developers, ovs dev
In-Reply-To: <1484267598-4493-1-git-send-email-lrichard@redhat.com>
On Thu, Jan 12, 2017 at 4:33 PM, Lance Richardson <lrichard@redhat.com> wrote:
> When executing conntrack actions on skbuffs with checksum mode
> CHECKSUM_COMPLETE, the checksum must be updated to account for
> header pushes and pulls. Otherwise we get "hw csum failure"
> logs similar to this (ICMP packet received on geneve tunnel
> via ixgbe NIC):
>
> [ 405.740065] genev_sys_6081: hw csum failure
> [ 405.740106] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G I 4.10.0-rc3+ #1
> [ 405.740108] Call Trace:
> [ 405.740110] <IRQ>
> [ 405.740113] dump_stack+0x63/0x87
> [ 405.740116] netdev_rx_csum_fault+0x3a/0x40
> [ 405.740118] __skb_checksum_complete+0xcf/0xe0
> [ 405.740120] nf_ip_checksum+0xc8/0xf0
> [ 405.740124] icmp_error+0x1de/0x351 [nf_conntrack_ipv4]
> [ 405.740132] nf_conntrack_in+0xe1/0x550 [nf_conntrack]
> [ 405.740137] ? find_bucket.isra.2+0x62/0x70 [openvswitch]
> [ 405.740143] __ovs_ct_lookup+0x95/0x980 [openvswitch]
> [ 405.740145] ? netif_rx_internal+0x44/0x110
> [ 405.740149] ovs_ct_execute+0x147/0x4b0 [openvswitch]
> [ 405.740153] do_execute_actions+0x22e/0xa70 [openvswitch]
> [ 405.740157] ovs_execute_actions+0x40/0x120 [openvswitch]
> [ 405.740161] ovs_dp_process_packet+0x84/0x120 [openvswitch]
> [ 405.740166] ovs_vport_receive+0x73/0xd0 [openvswitch]
> [ 405.740168] ? udp_rcv+0x1a/0x20
> [ 405.740170] ? ip_local_deliver_finish+0x93/0x1e0
> [ 405.740172] ? ip_local_deliver+0x6f/0xe0
> [ 405.740174] ? ip_rcv_finish+0x3a0/0x3a0
> [ 405.740176] ? ip_rcv_finish+0xdb/0x3a0
> [ 405.740177] ? ip_rcv+0x2a7/0x400
> [ 405.740180] ? __netif_receive_skb_core+0x970/0xa00
> [ 405.740185] netdev_frame_hook+0xd3/0x160 [openvswitch]
> [ 405.740187] __netif_receive_skb_core+0x1dc/0xa00
> [ 405.740194] ? ixgbe_clean_rx_irq+0x46d/0xa20 [ixgbe]
> [ 405.740197] __netif_receive_skb+0x18/0x60
> [ 405.740199] netif_receive_skb_internal+0x40/0xb0
> [ 405.740201] napi_gro_receive+0xcd/0x120
> [ 405.740204] gro_cell_poll+0x57/0x80 [geneve]
> [ 405.740206] net_rx_action+0x260/0x3c0
> [ 405.740209] __do_softirq+0xc9/0x28c
> [ 405.740211] irq_exit+0xd9/0xf0
> [ 405.740213] do_IRQ+0x51/0xd0
> [ 405.740215] common_interrupt+0x93/0x93
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Signed-off-by: Lance Richardson <lrichard@redhat.com>
Looks good.
Acked-by: Pravin B Shelar <pshelar@ovn.org>
Thanks.
^ 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