* Re: [PATCH bpf-next v2] tools: bpftool: add "prog run" subcommand to test-run programs
From: Y Song @ 2019-07-05 18:21 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
In-Reply-To: <20190705175433.22511-1-quentin.monnet@netronome.com>
On Fri, Jul 5, 2019 at 10:54 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> Add a new "bpftool prog run" subcommand to run a loaded program on input
> data (and possibly with input context) passed by the user.
>
> Print output data (and output context if relevant) into a file or into
> the console. Print return value and duration for the test run into the
> console.
>
> A "repeat" argument can be passed to run the program several times in a
> row.
>
> The command does not perform any kind of verification based on program
> type (Is this program type allowed to use an input context?) or on data
> consistency (Can I work with empty input data?), this is left to the
> kernel.
>
> Example invocation:
>
> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
> pinned /sys/fs/bpf/sample_ret0 \
> data_in - data_out - repeat 5
> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
> Return value: 0, duration (average): 260ns
>
> When one of data_in or ctx_in is "-", bpftool reads from standard input,
> in binary format. Other formats (JSON, hexdump) might be supported (via
> an optional command line keyword like "data_fmt_in") in the future if
> relevant, but this would require doing more parsing in bpftool.
>
> v2:
> - Fix argument names for function check_single_stdin(). (Yonghong)
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* [PATCH bpf-next 2/3] xdp: Refactor devmap allocation code for reuse
From: Toke Høiland-Jørgensen @ 2019-07-05 17:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, netdev, David Miller, Jesper Dangaard Brouer,
Jakub Kicinski, Björn Töpel
In-Reply-To: <156234940798.2378.9008707939063611210.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
The subsequent patch to add a new devmap sub-type can re-use much of the
initialisation and allocation code, so refactor it into separate functions.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
kernel/bpf/devmap.c | 137 +++++++++++++++++++++++++++++++--------------------
1 file changed, 84 insertions(+), 53 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index d83cf8ccc872..a2fe16362129 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -60,7 +60,7 @@ struct xdp_bulk_queue {
struct bpf_dtab_netdev {
struct net_device *dev; /* must be first member, due to tracepoint */
struct bpf_dtab *dtab;
- unsigned int bit;
+ unsigned int idx; /* keep track of map index for tracepoint */
struct xdp_bulk_queue __percpu *bulkq;
struct rcu_head rcu;
};
@@ -75,28 +75,22 @@ struct bpf_dtab {
static DEFINE_SPINLOCK(dev_map_lock);
static LIST_HEAD(dev_map_list);
-static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
+static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
+ bool check_memlock)
{
- struct bpf_dtab *dtab;
int err, cpu;
u64 cost;
- if (!capable(CAP_NET_ADMIN))
- return ERR_PTR(-EPERM);
-
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
- return ERR_PTR(-EINVAL);
+ return -EINVAL;
/* Lookup returns a pointer straight to dev->ifindex, so make sure the
* verifier prevents writes from the BPF side
*/
attr->map_flags |= BPF_F_RDONLY_PROG;
- dtab = kzalloc(sizeof(*dtab), GFP_USER);
- if (!dtab)
- return ERR_PTR(-ENOMEM);
bpf_map_init_from_attr(&dtab->map, attr);
@@ -107,9 +101,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
/* if map size is larger than memlock limit, reject it */
err = bpf_map_charge_init(&dtab->map.memory, cost);
if (err)
- goto free_dtab;
-
- err = -ENOMEM;
+ return -EINVAL;
dtab->flush_list = alloc_percpu(struct list_head);
if (!dtab->flush_list)
@@ -124,19 +116,38 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
if (!dtab->netdev_map)
goto free_percpu;
- spin_lock(&dev_map_lock);
- list_add_tail_rcu(&dtab->list, &dev_map_list);
- spin_unlock(&dev_map_lock);
-
- return &dtab->map;
+ return 0;
free_percpu:
free_percpu(dtab->flush_list);
free_charge:
bpf_map_charge_finish(&dtab->map.memory);
-free_dtab:
- kfree(dtab);
- return ERR_PTR(err);
+ return -ENOMEM;
+}
+
+static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
+{
+ struct bpf_dtab *dtab;
+ int err;
+
+ if (!capable(CAP_NET_ADMIN))
+ return ERR_PTR(-EPERM);
+
+ dtab = kzalloc(sizeof(*dtab), GFP_USER);
+ if (!dtab)
+ return ERR_PTR(-ENOMEM);
+
+ err = dev_map_init_map(dtab, attr, true);
+ if (err) {
+ kfree(dtab);
+ return ERR_PTR(err);
+ }
+
+ spin_lock(&dev_map_lock);
+ list_add_tail_rcu(&dtab->list, &dev_map_list);
+ spin_unlock(&dev_map_lock);
+
+ return &dtab->map;
}
static void dev_map_free(struct bpf_map *map)
@@ -235,7 +246,7 @@ static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags,
out:
bq->count = 0;
- trace_xdp_devmap_xmit(&obj->dtab->map, obj->bit,
+ trace_xdp_devmap_xmit(&obj->dtab->map, obj->idx,
sent, drops, bq->dev_rx, dev, err);
bq->dev_rx = NULL;
__list_del_clearprev(&bq->flush_node);
@@ -412,17 +423,52 @@ static int dev_map_delete_elem(struct bpf_map *map, void *key)
return 0;
}
-static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
- u64 map_flags)
+static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
+ struct bpf_dtab *dtab,
+ u32 ifindex,
+ unsigned int idx)
{
- struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
- struct net *net = current->nsproxy->net_ns;
gfp_t gfp = GFP_ATOMIC | __GFP_NOWARN;
+ struct bpf_dtab_netdev *dev;
+ struct xdp_bulk_queue *bq;
+ int cpu;
+
+ dev = kmalloc_node(sizeof(*dev), gfp, dtab->map.numa_node);
+ if (!dev)
+ return ERR_PTR(-ENOMEM);
+
+ dev->bulkq = __alloc_percpu_gfp(sizeof(*dev->bulkq),
+ sizeof(void *), gfp);
+ if (!dev->bulkq) {
+ kfree(dev);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ for_each_possible_cpu(cpu) {
+ bq = per_cpu_ptr(dev->bulkq, cpu);
+ bq->obj = dev;
+ }
+
+ dev->dev = dev_get_by_index(net, ifindex);
+ if (!dev->dev) {
+ free_percpu(dev->bulkq);
+ kfree(dev);
+ return ERR_PTR(-EINVAL);
+ }
+
+ dev->idx = idx;
+ dev->dtab = dtab;
+
+ return dev;
+}
+
+static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
+ void *key, void *value, u64 map_flags)
+{
+ struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
struct bpf_dtab_netdev *dev, *old_dev;
u32 ifindex = *(u32 *)value;
- struct xdp_bulk_queue *bq;
u32 i = *(u32 *)key;
- int cpu;
if (unlikely(map_flags > BPF_EXIST))
return -EINVAL;
@@ -434,31 +480,9 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
if (!ifindex) {
dev = NULL;
} else {
- dev = kmalloc_node(sizeof(*dev), gfp, map->numa_node);
- if (!dev)
- return -ENOMEM;
-
- dev->bulkq = __alloc_percpu_gfp(sizeof(*dev->bulkq),
- sizeof(void *), gfp);
- if (!dev->bulkq) {
- kfree(dev);
- return -ENOMEM;
- }
-
- for_each_possible_cpu(cpu) {
- bq = per_cpu_ptr(dev->bulkq, cpu);
- bq->obj = dev;
- }
-
- dev->dev = dev_get_by_index(net, ifindex);
- if (!dev->dev) {
- free_percpu(dev->bulkq);
- kfree(dev);
- return -EINVAL;
- }
-
- dev->bit = i;
- dev->dtab = dtab;
+ dev = __dev_map_alloc_node(net, dtab, ifindex, i);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
}
/* Use call_rcu() here to ensure rcu critical sections have completed
@@ -472,6 +496,13 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
return 0;
}
+static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
+ u64 map_flags)
+{
+ return __dev_map_update_elem(current->nsproxy->net_ns,
+ map, key, value, map_flags);
+}
+
const struct bpf_map_ops dev_map_ops = {
.map_alloc = dev_map_alloc,
.map_free = dev_map_free,
^ permalink raw reply related
* [PATCH bpf-next 3/3] xdp: Add devmap_hash map type for looking up devices by hashed index
From: Toke Høiland-Jørgensen @ 2019-07-05 17:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, netdev, David Miller, Jesper Dangaard Brouer,
Jakub Kicinski, Björn Töpel
In-Reply-To: <156234940798.2378.9008707939063611210.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
A common pattern when using xdp_redirect_map() is to create a device map
where the lookup key is simply ifindex. Because device maps are arrays,
this leaves holes in the map, and the map has to be sized to fit the
largest ifindex, regardless of how many devices actually are actually
needed in the map.
This patch adds a second type of device map where the key is looked up
using a hashmap, instead of being used as an array index. This allows maps
to be densely packed, so they can be smaller.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/bpf.h | 7 +
include/linux/bpf_types.h | 1
include/trace/events/xdp.h | 3
include/uapi/linux/bpf.h | 7 +
kernel/bpf/devmap.c | 192 +++++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 2
net/core/filter.c | 9 +
tools/bpf/bpftool/map.c | 1
tools/include/uapi/linux/bpf.h | 7 +
tools/lib/bpf/libbpf_probes.c | 1
tools/testing/selftests/bpf/test_maps.c | 16 +++
11 files changed, 237 insertions(+), 9 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bfdb54dd2ad1..f9a506147c8a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -713,6 +713,7 @@ struct xdp_buff;
struct sk_buff;
struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
+struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key);
void __dev_map_flush(struct bpf_map *map);
int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
struct net_device *dev_rx);
@@ -799,6 +800,12 @@ static inline struct net_device *__dev_map_lookup_elem(struct bpf_map *map,
return NULL;
}
+static inline struct net_device *__dev_map_hash_lookup_elem(struct bpf_map *map,
+ u32 key)
+{
+ return NULL;
+}
+
static inline void __dev_map_flush(struct bpf_map *map)
{
}
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index eec5aeeeaf92..36a9c2325176 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -62,6 +62,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY_OF_MAPS, array_of_maps_map_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_HASH_OF_MAPS, htab_of_maps_map_ops)
#ifdef CONFIG_NET
BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
+BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP_HASH, dev_map_hash_ops)
BPF_MAP_TYPE(BPF_MAP_TYPE_SK_STORAGE, sk_storage_map_ops)
#if defined(CONFIG_BPF_STREAM_PARSER)
BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 68899fdc985b..8c8420230a10 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -175,7 +175,8 @@ struct _bpf_dtab_netdev {
#endif /* __DEVMAP_OBJ_TYPE */
#define devmap_ifindex(fwd, map) \
- ((map->map_type == BPF_MAP_TYPE_DEVMAP) ? \
+ ((map->map_type == BPF_MAP_TYPE_DEVMAP || \
+ map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) ? \
((struct _bpf_dtab_netdev *)fwd)->dev->ifindex : 0)
#define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx) \
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ead27aebf491..05ce55dd366a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -134,6 +134,7 @@ enum bpf_map_type {
BPF_MAP_TYPE_QUEUE,
BPF_MAP_TYPE_STACK,
BPF_MAP_TYPE_SK_STORAGE,
+ BPF_MAP_TYPE_DEVMAP_HASH,
};
/* Note that tracing related programs such as
@@ -879,14 +880,14 @@ union bpf_attr {
*
* int ret;
* struct bpf_tunnel_key key = {};
- *
+ *
* ret = bpf_skb_get_tunnel_key(skb, &key, sizeof(key), 0);
* if (ret < 0)
* return TC_ACT_SHOT; // drop packet
- *
+ *
* if (key.remote_ipv4 != 0x0a000001)
* return TC_ACT_SHOT; // drop packet
- *
+ *
* return TC_ACT_OK; // accept packet
*
* This interface can also be used with all encapsulation devices
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index a2fe16362129..341af02f049d 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -37,6 +37,12 @@
* notifier hook walks the map we know that new dev references can not be
* added by the user because core infrastructure ensures dev_get_by_index()
* calls will fail at this point.
+ *
+ * The devmap_hash type is a map type which interprets keys as ifindexes and
+ * indexes these using a hashmap. This allows maps that use ifindex as key to be
+ * densely packed instead of having holes in the lookup array for unused
+ * ifindexes. The setup and packet enqueue/send code is shared between the two
+ * types of devmap; only the lookup and insertion is different.
*/
#include <linux/bpf.h>
#include <net/xdp.h>
@@ -59,6 +65,7 @@ struct xdp_bulk_queue {
struct bpf_dtab_netdev {
struct net_device *dev; /* must be first member, due to tracepoint */
+ struct hlist_node index_hlist;
struct bpf_dtab *dtab;
unsigned int idx; /* keep track of map index for tracepoint */
struct xdp_bulk_queue __percpu *bulkq;
@@ -70,11 +77,29 @@ struct bpf_dtab {
struct bpf_dtab_netdev **netdev_map;
struct list_head __percpu *flush_list;
struct list_head list;
+
+ /* these are only used for DEVMAP_HASH type maps */
+ unsigned int items;
+ struct hlist_head *dev_index_head;
+ spinlock_t index_lock;
};
static DEFINE_SPINLOCK(dev_map_lock);
static LIST_HEAD(dev_map_list);
+static struct hlist_head *dev_map_create_hash(void)
+{
+ int i;
+ struct hlist_head *hash;
+
+ hash = kmalloc_array(NETDEV_HASHENTRIES, sizeof(*hash), GFP_KERNEL);
+ if (hash != NULL)
+ for (i = 0; i < NETDEV_HASHENTRIES; i++)
+ INIT_HLIST_HEAD(&hash[i]);
+
+ return hash;
+}
+
static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
bool check_memlock)
{
@@ -98,6 +123,9 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
cost += sizeof(struct list_head) * num_possible_cpus();
+ if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH)
+ cost += sizeof(struct hlist_head) * NETDEV_HASHENTRIES;
+
/* if map size is larger than memlock limit, reject it */
err = bpf_map_charge_init(&dtab->map.memory, cost);
if (err)
@@ -116,8 +144,18 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr,
if (!dtab->netdev_map)
goto free_percpu;
+ if (attr->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
+ dtab->dev_index_head = dev_map_create_hash();
+ if (!dtab->dev_index_head)
+ goto free_map_area;
+
+ spin_lock_init(&dtab->index_lock);
+ }
+
return 0;
+free_map_area:
+ bpf_map_area_free(dtab->netdev_map);
free_percpu:
free_percpu(dtab->flush_list);
free_charge:
@@ -199,6 +237,7 @@ static void dev_map_free(struct bpf_map *map)
free_percpu(dtab->flush_list);
bpf_map_area_free(dtab->netdev_map);
+ kfree(dtab->dev_index_head);
kfree(dtab);
}
@@ -219,6 +258,70 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
return 0;
}
+static inline struct hlist_head *dev_map_index_hash(struct bpf_dtab *dtab,
+ int idx)
+{
+ return &dtab->dev_index_head[idx & (NETDEV_HASHENTRIES - 1)];
+}
+
+struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
+{
+ struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
+ struct hlist_head *head = dev_map_index_hash(dtab, key);
+ struct bpf_dtab_netdev *dev;
+
+ hlist_for_each_entry_rcu(dev, head, index_hlist)
+ if (dev->idx == key)
+ return dev;
+
+ return NULL;
+}
+
+static int dev_map_hash_get_next_key(struct bpf_map *map, void *key,
+ void *next_key)
+{
+ struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
+ u32 idx, *next = next_key;
+ struct bpf_dtab_netdev *dev, *next_dev;
+ struct hlist_head *head;
+ int i = 0;
+
+ if (!key)
+ goto find_first;
+
+ idx = *(u32 *)key;
+
+ dev = __dev_map_hash_lookup_elem(map, idx);
+ if (!dev)
+ goto find_first;
+
+ next_dev = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(&dev->index_hlist)),
+ struct bpf_dtab_netdev, index_hlist);
+
+ if (next_dev) {
+ *next = next_dev->idx;
+ return 0;
+ }
+
+ i = idx & (NETDEV_HASHENTRIES - 1);
+ i++;
+
+ find_first:
+ for (; i < NETDEV_HASHENTRIES; i++) {
+ head = dev_map_index_hash(dtab, i);
+
+ next_dev = hlist_entry_safe(rcu_dereference_raw(hlist_first_rcu(head)),
+ struct bpf_dtab_netdev,
+ index_hlist);
+ if (next_dev) {
+ *next = next_dev->idx;
+ return 0;
+ }
+ }
+
+ return -ENOENT;
+}
+
static int bq_xmit_all(struct xdp_bulk_queue *bq, u32 flags,
bool in_napi_ctx)
{
@@ -374,6 +477,15 @@ static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
return dev ? &dev->ifindex : NULL;
}
+static void *dev_map_hash_lookup_elem(struct bpf_map *map, void *key)
+{
+ struct bpf_dtab_netdev *obj = __dev_map_hash_lookup_elem(map,
+ *(u32 *)key);
+ struct net_device *dev = obj ? obj->dev : NULL;
+
+ return dev ? &dev->ifindex : NULL;
+}
+
static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
{
if (dev->dev->netdev_ops->ndo_xdp_xmit) {
@@ -423,6 +535,27 @@ static int dev_map_delete_elem(struct bpf_map *map, void *key)
return 0;
}
+static int dev_map_hash_delete_elem(struct bpf_map *map, void *key)
+{
+ struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
+ struct bpf_dtab_netdev *old_dev;
+ int k = *(u32 *)key;
+
+ old_dev = __dev_map_hash_lookup_elem(map, k);
+ if (!old_dev)
+ return 0;
+
+ spin_lock(&dtab->index_lock);
+ if (!hlist_unhashed(&old_dev->index_hlist)) {
+ dtab->items--;
+ hlist_del_init_rcu(&old_dev->index_hlist);
+ }
+ spin_unlock(&dtab->index_lock);
+
+ call_rcu(&old_dev->rcu, __dev_map_entry_free);
+ return 0;
+}
+
static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
struct bpf_dtab *dtab,
u32 ifindex,
@@ -503,6 +636,55 @@ static int dev_map_update_elem(struct bpf_map *map, void *key, void *value,
map, key, value, map_flags);
}
+static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
+ void *key, void *value, u64 map_flags)
+{
+ struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
+ struct bpf_dtab_netdev *dev, *old_dev;
+ u32 ifindex = *(u32 *)value;
+ u32 idx = *(u32 *)key;
+
+ if (unlikely(map_flags > BPF_EXIST || !ifindex))
+ return -EINVAL;
+
+ old_dev = __dev_map_hash_lookup_elem(map, idx);
+ if (old_dev && (map_flags & BPF_NOEXIST))
+ return -EEXIST;
+
+ dev = __dev_map_alloc_node(net, dtab, ifindex, idx);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ spin_lock(&dtab->index_lock);
+
+ if (old_dev) {
+ hlist_del_rcu(&old_dev->index_hlist);
+ } else {
+ if (dtab->items >= dtab->map.max_entries) {
+ spin_unlock(&dtab->index_lock);
+ call_rcu(&dev->rcu, __dev_map_entry_free);
+ return -ENOSPC;
+ }
+ dtab->items++;
+ }
+
+ hlist_add_head_rcu(&dev->index_hlist,
+ dev_map_index_hash(dtab, idx));
+ spin_unlock(&dtab->index_lock);
+
+ if (old_dev)
+ call_rcu(&old_dev->rcu, __dev_map_entry_free);
+
+ return 0;
+}
+
+static int dev_map_hash_update_elem(struct bpf_map *map, void *key, void *value,
+ u64 map_flags)
+{
+ return __dev_map_hash_update_elem(current->nsproxy->net_ns,
+ map, key, value, map_flags);
+}
+
const struct bpf_map_ops dev_map_ops = {
.map_alloc = dev_map_alloc,
.map_free = dev_map_free,
@@ -513,6 +695,16 @@ const struct bpf_map_ops dev_map_ops = {
.map_check_btf = map_check_no_btf,
};
+const struct bpf_map_ops dev_map_hash_ops = {
+ .map_alloc = dev_map_alloc,
+ .map_free = dev_map_free,
+ .map_get_next_key = dev_map_hash_get_next_key,
+ .map_lookup_elem = dev_map_hash_lookup_elem,
+ .map_update_elem = dev_map_hash_update_elem,
+ .map_delete_elem = dev_map_hash_delete_elem,
+ .map_check_btf = map_check_no_btf,
+};
+
static int dev_map_notification(struct notifier_block *notifier,
ulong event, void *ptr)
{
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a2e763703c30..231b9e22827c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3460,6 +3460,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
goto error;
break;
case BPF_MAP_TYPE_DEVMAP:
+ case BPF_MAP_TYPE_DEVMAP_HASH:
if (func_id != BPF_FUNC_redirect_map &&
func_id != BPF_FUNC_map_lookup_elem)
goto error;
@@ -3542,6 +3543,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
break;
case BPF_FUNC_redirect_map:
if (map->map_type != BPF_MAP_TYPE_DEVMAP &&
+ map->map_type != BPF_MAP_TYPE_DEVMAP_HASH &&
map->map_type != BPF_MAP_TYPE_CPUMAP &&
map->map_type != BPF_MAP_TYPE_XSKMAP)
goto error;
diff --git a/net/core/filter.c b/net/core/filter.c
index 089aaea0ccc6..276961c4e0d4 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3517,7 +3517,8 @@ static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
int err;
switch (map->map_type) {
- case BPF_MAP_TYPE_DEVMAP: {
+ case BPF_MAP_TYPE_DEVMAP:
+ case BPF_MAP_TYPE_DEVMAP_HASH: {
struct bpf_dtab_netdev *dst = fwd;
err = dev_map_enqueue(dst, xdp, dev_rx);
@@ -3554,6 +3555,7 @@ void xdp_do_flush_map(void)
if (map) {
switch (map->map_type) {
case BPF_MAP_TYPE_DEVMAP:
+ case BPF_MAP_TYPE_DEVMAP_HASH:
__dev_map_flush(map);
break;
case BPF_MAP_TYPE_CPUMAP:
@@ -3574,6 +3576,8 @@ static inline void *__xdp_map_lookup_elem(struct bpf_map *map, u32 index)
switch (map->map_type) {
case BPF_MAP_TYPE_DEVMAP:
return __dev_map_lookup_elem(map, index);
+ case BPF_MAP_TYPE_DEVMAP_HASH:
+ return __dev_map_hash_lookup_elem(map, index);
case BPF_MAP_TYPE_CPUMAP:
return __cpu_map_lookup_elem(map, index);
case BPF_MAP_TYPE_XSKMAP:
@@ -3655,7 +3659,8 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
ri->tgt_value = NULL;
WRITE_ONCE(ri->map, NULL);
- if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
+ if (map->map_type == BPF_MAP_TYPE_DEVMAP ||
+ map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
struct bpf_dtab_netdev *dst = fwd;
err = dev_map_generic_redirect(dst, skb, xdp_prog);
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 5da5a7311f13..c345f819b840 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -37,6 +37,7 @@ const char * const map_type_name[] = {
[BPF_MAP_TYPE_ARRAY_OF_MAPS] = "array_of_maps",
[BPF_MAP_TYPE_HASH_OF_MAPS] = "hash_of_maps",
[BPF_MAP_TYPE_DEVMAP] = "devmap",
+ [BPF_MAP_TYPE_DEVMAP_HASH] = "devmap_hash",
[BPF_MAP_TYPE_SOCKMAP] = "sockmap",
[BPF_MAP_TYPE_CPUMAP] = "cpumap",
[BPF_MAP_TYPE_XSKMAP] = "xskmap",
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index cecf42c871d4..a5551302709b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -134,6 +134,7 @@ enum bpf_map_type {
BPF_MAP_TYPE_QUEUE,
BPF_MAP_TYPE_STACK,
BPF_MAP_TYPE_SK_STORAGE,
+ BPF_MAP_TYPE_DEVMAP_HASH,
};
/* Note that tracing related programs such as
@@ -879,14 +880,14 @@ union bpf_attr {
*
* int ret;
* struct bpf_tunnel_key key = {};
- *
+ *
* ret = bpf_skb_get_tunnel_key(skb, &key, sizeof(key), 0);
* if (ret < 0)
* return TC_ACT_SHOT; // drop packet
- *
+ *
* if (key.remote_ipv4 != 0x0a000001)
* return TC_ACT_SHOT; // drop packet
- *
+ *
* return TC_ACT_OK; // accept packet
*
* This interface can also be used with all encapsulation devices
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index ace1a0708d99..4b0b0364f5fc 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -244,6 +244,7 @@ bool bpf_probe_map_type(enum bpf_map_type map_type, __u32 ifindex)
case BPF_MAP_TYPE_ARRAY_OF_MAPS:
case BPF_MAP_TYPE_HASH_OF_MAPS:
case BPF_MAP_TYPE_DEVMAP:
+ case BPF_MAP_TYPE_DEVMAP_HASH:
case BPF_MAP_TYPE_SOCKMAP:
case BPF_MAP_TYPE_CPUMAP:
case BPF_MAP_TYPE_XSKMAP:
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index a3fbc571280a..086319caf2d9 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -508,6 +508,21 @@ static void test_devmap(unsigned int task, void *data)
close(fd);
}
+static void test_devmap_hash(unsigned int task, void *data)
+{
+ int fd;
+ __u32 key, value;
+
+ fd = bpf_create_map(BPF_MAP_TYPE_DEVMAP_HASH, sizeof(key), sizeof(value),
+ 2, 0);
+ if (fd < 0) {
+ printf("Failed to create devmap_hash '%s'!\n", strerror(errno));
+ exit(1);
+ }
+
+ close(fd);
+}
+
static void test_queuemap(unsigned int task, void *data)
{
const int MAP_SIZE = 32;
@@ -1675,6 +1690,7 @@ static void run_all_tests(void)
test_arraymap_percpu_many_keys();
test_devmap(0, NULL);
+ test_devmap_hash(0, NULL);
test_sockmap(0, NULL);
test_map_large();
^ permalink raw reply related
* [PATCH bpf-next 1/3] include/bpf.h: Remove map_insert_ctx() stubs
From: Toke Høiland-Jørgensen @ 2019-07-05 17:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, netdev, David Miller, Jesper Dangaard Brouer,
Jakub Kicinski, Björn Töpel
In-Reply-To: <156234940798.2378.9008707939063611210.stgit@alrua-x1>
From: Toke Høiland-Jørgensen <toke@redhat.com>
When we changed the device and CPU maps to use linked lists instead of
bitmaps, we also removed the need for the map_insert_ctx() helpers to keep
track of the bitmaps inside each map. However, it seems I forgot to remove
the function definitions stubs, so remove those here.
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
include/linux/bpf.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 18f4cc2c6acd..bfdb54dd2ad1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -713,7 +713,6 @@ struct xdp_buff;
struct sk_buff;
struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
-void __dev_map_insert_ctx(struct bpf_map *map, u32 index);
void __dev_map_flush(struct bpf_map *map);
int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
struct net_device *dev_rx);
@@ -721,7 +720,6 @@ int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
struct bpf_prog *xdp_prog);
struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
-void __cpu_map_insert_ctx(struct bpf_map *map, u32 index);
void __cpu_map_flush(struct bpf_map *map);
int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
struct net_device *dev_rx);
@@ -801,10 +799,6 @@ static inline struct net_device *__dev_map_lookup_elem(struct bpf_map *map,
return NULL;
}
-static inline void __dev_map_insert_ctx(struct bpf_map *map, u32 index)
-{
-}
-
static inline void __dev_map_flush(struct bpf_map *map)
{
}
@@ -834,10 +828,6 @@ struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
return NULL;
}
-static inline void __cpu_map_insert_ctx(struct bpf_map *map, u32 index)
-{
-}
-
static inline void __cpu_map_flush(struct bpf_map *map)
{
}
^ permalink raw reply related
* [PATCH bpf-next 0/3] xdp: Add devmap_hash map type
From: Toke Høiland-Jørgensen @ 2019-07-05 17:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, netdev, David Miller, Jesper Dangaard Brouer,
Jakub Kicinski, Björn Töpel
This series adds a new map type, devmap_hash, that works like the existing
devmap type, but using a hash-based indexing scheme. This is useful for the use
case where a devmap is indexed by ifindex (for instance for use with the routing
table lookup helper). For this use case, the regular devmap needs to be sized
after the maximum ifindex number, not the number of devices in it. A hash-based
indexing scheme makes it possible to size the map after the number of devices it
should contain instead.
This was previously part of my patch series that also turned the regular
bpf_redirect() helper into a map-based one; for this series I just pulled out
the patches that introduced the new map type.
Changelog:
Changes to these patches since the previous series:
- Rebase on top of the other devmap changes (makes this one simpler!)
- Don't enforce key==val, but allow arbitrary indexes.
- Rename the type to devmap_hash to reflect the fact that it's just a hashmap now.
---
Toke Høiland-Jørgensen (3):
include/bpf.h: Remove map_insert_ctx() stubs
xdp: Refactor devmap allocation code for reuse
xdp: Add devmap_hash map type for looking up devices by hashed index
include/linux/bpf.h | 11 -
include/linux/bpf_types.h | 1
include/trace/events/xdp.h | 3
include/uapi/linux/bpf.h | 7 -
kernel/bpf/devmap.c | 325 ++++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 2
net/core/filter.c | 9 +
tools/bpf/bpftool/map.c | 1
tools/include/uapi/linux/bpf.h | 7 -
tools/lib/bpf/libbpf_probes.c | 1
tools/testing/selftests/bpf/test_maps.c | 16 ++
11 files changed, 316 insertions(+), 67 deletions(-)
^ permalink raw reply
* [PATCH bpf-next v2] tools: bpftool: add "prog run" subcommand to test-run programs
From: Quentin Monnet @ 2019-07-05 17:54 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: bpf, netdev, oss-drivers, Quentin Monnet, Yonghong Song
Add a new "bpftool prog run" subcommand to run a loaded program on input
data (and possibly with input context) passed by the user.
Print output data (and output context if relevant) into a file or into
the console. Print return value and duration for the test run into the
console.
A "repeat" argument can be passed to run the program several times in a
row.
The command does not perform any kind of verification based on program
type (Is this program type allowed to use an input context?) or on data
consistency (Can I work with empty input data?), this is left to the
kernel.
Example invocation:
# perl -e 'print "\x0" x 14' | ./bpftool prog run \
pinned /sys/fs/bpf/sample_ret0 \
data_in - data_out - repeat 5
0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
Return value: 0, duration (average): 260ns
When one of data_in or ctx_in is "-", bpftool reads from standard input,
in binary format. Other formats (JSON, hexdump) might be supported (via
an optional command line keyword like "data_fmt_in") in the future if
relevant, but this would require doing more parsing in bpftool.
v2:
- Fix argument names for function check_single_stdin(). (Yonghong)
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
.../bpftool/Documentation/bpftool-prog.rst | 34 ++
tools/bpf/bpftool/bash-completion/bpftool | 28 +-
tools/bpf/bpftool/main.c | 29 ++
tools/bpf/bpftool/main.h | 1 +
tools/bpf/bpftool/prog.c | 348 +++++++++++++++++-
tools/include/linux/sizes.h | 48 +++
6 files changed, 485 insertions(+), 3 deletions(-)
create mode 100644 tools/include/linux/sizes.h
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 1df637f85f94..7a374b3c851d 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -29,6 +29,7 @@ PROG COMMANDS
| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
| **bpftool** **prog tracelog**
+| **bpftool** **prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
| **bpftool** **prog help**
|
| *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -146,6 +147,39 @@ DESCRIPTION
streaming data from BPF programs to user space, one can use
perf events (see also **bpftool-map**\ (8)).
+ **bpftool prog run** *PROG* **data_in** *FILE* [**data_out** *FILE* [**data_size_out** *L*]] [**ctx_in** *FILE* [**ctx_out** *FILE* [**ctx_size_out** *M*]]] [**repeat** *N*]
+ Run BPF program *PROG* in the kernel testing infrastructure
+ for BPF, meaning that the program works on the data and
+ context provided by the user, and not on actual packets or
+ monitored functions etc. Return value and duration for the
+ test run are printed out to the console.
+
+ Input data is read from the *FILE* passed with **data_in**.
+ If this *FILE* is "**-**", input data is read from standard
+ input. Input context, if any, is read from *FILE* passed with
+ **ctx_in**. Again, "**-**" can be used to read from standard
+ input, but only if standard input is not already in use for
+ input data. If a *FILE* is passed with **data_out**, output
+ data is written to that file. Similarly, output context is
+ written to the *FILE* passed with **ctx_out**. For both
+ output flows, "**-**" can be used to print to the standard
+ output (as plain text, or JSON if relevant option was
+ passed). If output keywords are omitted, output data and
+ context are discarded. Keywords **data_size_out** and
+ **ctx_size_out** are used to pass the size (in bytes) for the
+ output buffers to the kernel, although the default of 32 kB
+ should be more than enough for most cases.
+
+ Keyword **repeat** is used to indicate the number of
+ consecutive runs to perform. Note that output data and
+ context printed to files correspond to the last of those
+ runs. The duration printed out at the end of the runs is an
+ average over all runs performed by the command.
+
+ Not all program types support test run. Among those which do,
+ not all of them can take the **ctx_in**/**ctx_out**
+ arguments. bpftool does not perform checks on program types.
+
**bpftool prog help**
Print short help message.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..965a8658cca3 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -408,10 +408,34 @@ _bpftool()
tracelog)
return 0
;;
+ run)
+ if [[ ${#words[@]} -lt 5 ]]; then
+ _filedir
+ return 0
+ fi
+ case $prev in
+ id)
+ _bpftool_get_prog_ids
+ return 0
+ ;;
+ data_in|data_out|ctx_in|ctx_out)
+ _filedir
+ return 0
+ ;;
+ repeat|data_size_out|ctx_size_out)
+ return 0
+ ;;
+ *)
+ _bpftool_once_attr 'data_in data_out data_size_out \
+ ctx_in ctx_out ctx_size_out repeat'
+ return 0
+ ;;
+ esac
+ ;;
*)
[[ $prev == $object ]] && \
- COMPREPLY=( $( compgen -W 'dump help pin attach detach load \
- show list tracelog' -- "$cur" ) )
+ COMPREPLY=( $( compgen -W 'dump help pin attach detach \
+ load show list tracelog run' -- "$cur" ) )
;;
esac
;;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 4879f6395c7e..e916ff25697f 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -117,6 +117,35 @@ bool is_prefix(const char *pfx, const char *str)
return !memcmp(str, pfx, strlen(pfx));
}
+/* Last argument MUST be NULL pointer */
+int detect_common_prefix(const char *arg, ...)
+{
+ unsigned int count = 0;
+ const char *ref;
+ char msg[256];
+ va_list ap;
+
+ snprintf(msg, sizeof(msg), "ambiguous prefix: '%s' could be '", arg);
+ va_start(ap, arg);
+ while ((ref = va_arg(ap, const char *))) {
+ if (!is_prefix(arg, ref))
+ continue;
+ count++;
+ if (count > 1)
+ strncat(msg, "' or '", sizeof(msg) - strlen(msg) - 1);
+ strncat(msg, ref, sizeof(msg) - strlen(msg) - 1);
+ }
+ va_end(ap);
+ strncat(msg, "'", sizeof(msg) - strlen(msg) - 1);
+
+ if (count >= 2) {
+ p_err(msg);
+ return -1;
+ }
+
+ return 0;
+}
+
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
{
unsigned char *data = arg;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 9c5d9c80f71e..3ef0d9051e10 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -101,6 +101,7 @@ void p_err(const char *fmt, ...);
void p_info(const char *fmt, ...);
bool is_prefix(const char *pfx, const char *str);
+int detect_common_prefix(const char *arg, ...);
void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep);
void usage(void) __noreturn;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 9b0db5d14e31..66f04a4846a5 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -15,6 +15,7 @@
#include <sys/stat.h>
#include <linux/err.h>
+#include <linux/sizes.h>
#include <bpf.h>
#include <btf.h>
@@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
return 0;
}
+static int check_single_stdin(char *file_data_in, char *file_ctx_in)
+{
+ if (file_data_in && file_ctx_in &&
+ !strcmp(file_data_in, "-") && !strcmp(file_ctx_in, "-")) {
+ p_err("cannot use standard input for both data_in and ctx_in");
+ return -1;
+ }
+
+ return 0;
+}
+
+static int get_run_data(const char *fname, void **data_ptr, unsigned int *size)
+{
+ size_t block_size = 256;
+ size_t buf_size = block_size;
+ size_t nb_read = 0;
+ void *tmp;
+ FILE *f;
+
+ if (!fname) {
+ *data_ptr = NULL;
+ *size = 0;
+ return 0;
+ }
+
+ if (!strcmp(fname, "-"))
+ f = stdin;
+ else
+ f = fopen(fname, "r");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ *data_ptr = malloc(block_size);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_fclose;
+ }
+
+ while ((nb_read += fread(*data_ptr + nb_read, 1, block_size, f))) {
+ if (feof(f))
+ break;
+ if (ferror(f)) {
+ p_err("failed to read data_in/ctx_in from %s: %s",
+ fname, strerror(errno));
+ goto err_free;
+ }
+ if (nb_read > buf_size - block_size) {
+ if (buf_size == UINT32_MAX) {
+ p_err("data_in/ctx_in is too long (max: %d)",
+ UINT32_MAX);
+ goto err_free;
+ }
+ /* No space for fread()-ing next chunk; realloc() */
+ buf_size *= 2;
+ tmp = realloc(*data_ptr, buf_size);
+ if (!tmp) {
+ p_err("failed to reallocate data_in/ctx_in: %s",
+ strerror(errno));
+ goto err_free;
+ }
+ *data_ptr = tmp;
+ }
+ }
+ if (f != stdin)
+ fclose(f);
+
+ *size = nb_read;
+ return 0;
+
+err_free:
+ free(*data_ptr);
+ *data_ptr = NULL;
+err_fclose:
+ if (f != stdin)
+ fclose(f);
+ return -1;
+}
+
+static void hex_print(void *data, unsigned int size, FILE *f)
+{
+ size_t i, j;
+ char c;
+
+ for (i = 0; i < size; i += 16) {
+ /* Row offset */
+ fprintf(f, "%07zx\t", i);
+
+ /* Hexadecimal values */
+ for (j = i; j < i + 16 && j < size; j++)
+ fprintf(f, "%02x%s", *(uint8_t *)(data + j),
+ j % 2 ? " " : "");
+ for (; j < i + 16; j++)
+ fprintf(f, " %s", j % 2 ? " " : "");
+
+ /* ASCII values (if relevant), '.' otherwise */
+ fprintf(f, "| ");
+ for (j = i; j < i + 16 && j < size; j++) {
+ c = *(char *)(data + j);
+ if (c < ' ' || c > '~')
+ c = '.';
+ fprintf(f, "%c%s", c, j == i + 7 ? " " : "");
+ }
+
+ fprintf(f, "\n");
+ }
+}
+
+static int
+print_run_output(void *data, unsigned int size, const char *fname,
+ const char *json_key)
+{
+ size_t nb_written;
+ FILE *f;
+
+ if (!fname)
+ return 0;
+
+ if (!strcmp(fname, "-")) {
+ f = stdout;
+ if (json_output) {
+ jsonw_name(json_wtr, json_key);
+ print_data_json(data, size);
+ } else {
+ hex_print(data, size, f);
+ }
+ return 0;
+ }
+
+ f = fopen(fname, "w");
+ if (!f) {
+ p_err("failed to open %s: %s", fname, strerror(errno));
+ return -1;
+ }
+
+ nb_written = fwrite(data, 1, size, f);
+ fclose(f);
+ if (nb_written != size) {
+ p_err("failed to write output data/ctx: %s", strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int alloc_run_data(void **data_ptr, unsigned int size_out)
+{
+ *data_ptr = calloc(size_out, 1);
+ if (!*data_ptr) {
+ p_err("failed to allocate memory for output data/ctx: %s",
+ strerror(errno));
+ return -1;
+ }
+
+ return 0;
+}
+
+static int do_run(int argc, char **argv)
+{
+ char *data_fname_in = NULL, *data_fname_out = NULL;
+ char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
+ struct bpf_prog_test_run_attr test_attr = {0};
+ const unsigned int default_size = SZ_32K;
+ void *data_in = NULL, *data_out = NULL;
+ void *ctx_in = NULL, *ctx_out = NULL;
+ unsigned int repeat = 1;
+ int fd, err;
+
+ if (!REQ_ARGS(4))
+ return -1;
+
+ fd = prog_parse_fd(&argc, &argv);
+ if (fd < 0)
+ return -1;
+
+ while (argc) {
+ if (detect_common_prefix(*argv, "data_in", "data_out",
+ "data_size_out", NULL))
+ return -1;
+ if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
+ "ctx_size_out", NULL))
+ return -1;
+
+ if (is_prefix(*argv, "data_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_in = GET_ARG();
+ if (check_single_stdin(data_fname_in, ctx_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "data_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ data_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "data_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.data_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output data size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "ctx_in")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_in = GET_ARG();
+ if (check_single_stdin(data_fname_in, ctx_fname_in))
+ return -1;
+ } else if (is_prefix(*argv, "ctx_out")) {
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ ctx_fname_out = GET_ARG();
+ } else if (is_prefix(*argv, "ctx_size_out")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ test_attr.ctx_size_out = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as output context size",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else if (is_prefix(*argv, "repeat")) {
+ char *endptr;
+
+ NEXT_ARG();
+ if (!REQ_ARGS(1))
+ return -1;
+
+ repeat = strtoul(*argv, &endptr, 0);
+ if (*endptr) {
+ p_err("can't parse %s as repeat number",
+ *argv);
+ return -1;
+ }
+ NEXT_ARG();
+ } else {
+ p_err("expected no more arguments, 'data_in', 'data_out', 'data_size_out', 'ctx_in', 'ctx_out', 'ctx_size_out' or 'repeat', got: '%s'?",
+ *argv);
+ return -1;
+ }
+ }
+
+ err = get_run_data(data_fname_in, &data_in, &test_attr.data_size_in);
+ if (err)
+ return -1;
+
+ if (data_in) {
+ if (!test_attr.data_size_out)
+ test_attr.data_size_out = default_size;
+ err = alloc_run_data(&data_out, test_attr.data_size_out);
+ if (err)
+ goto free_data_in;
+ }
+
+ err = get_run_data(ctx_fname_in, &ctx_in, &test_attr.ctx_size_in);
+ if (err)
+ goto free_data_out;
+
+ if (ctx_in) {
+ if (!test_attr.ctx_size_out)
+ test_attr.ctx_size_out = default_size;
+ err = alloc_run_data(&ctx_out, test_attr.ctx_size_out);
+ if (err)
+ goto free_ctx_in;
+ }
+
+ test_attr.prog_fd = fd;
+ test_attr.repeat = repeat;
+ test_attr.data_in = data_in;
+ test_attr.data_out = data_out;
+ test_attr.ctx_in = ctx_in;
+ test_attr.ctx_out = ctx_out;
+
+ err = bpf_prog_test_run_xattr(&test_attr);
+ if (err) {
+ p_err("failed to run program: %s", strerror(errno));
+ goto free_ctx_out;
+ }
+
+ err = 0;
+
+ if (json_output)
+ jsonw_start_object(json_wtr); /* root */
+
+ /* Do not exit on errors occurring when printing output data/context,
+ * we still want to print return value and duration for program run.
+ */
+ if (test_attr.data_size_out)
+ err += print_run_output(test_attr.data_out,
+ test_attr.data_size_out,
+ data_fname_out, "data_out");
+ if (test_attr.ctx_size_out)
+ err += print_run_output(test_attr.ctx_out,
+ test_attr.ctx_size_out,
+ ctx_fname_out, "ctx_out");
+
+ if (json_output) {
+ jsonw_uint_field(json_wtr, "retval", test_attr.retval);
+ jsonw_uint_field(json_wtr, "duration", test_attr.duration);
+ jsonw_end_object(json_wtr); /* root */
+ } else {
+ fprintf(stdout, "Return value: %u, duration%s: %uns\n",
+ test_attr.retval,
+ repeat > 1 ? " (average)" : "", test_attr.duration);
+ }
+
+free_ctx_out:
+ free(ctx_out);
+free_ctx_in:
+ free(ctx_in);
+free_data_out:
+ free(data_out);
+free_data_in:
+ free(data_in);
+
+ return err;
+}
+
static int load_with_options(int argc, char **argv, bool first_prog_only)
{
struct bpf_object_load_attr load_attr = { 0 };
@@ -1058,6 +1397,11 @@ static int do_help(int argc, char **argv)
" [pinmaps MAP_DIR]\n"
" %s %s attach PROG ATTACH_TYPE [MAP]\n"
" %s %s detach PROG ATTACH_TYPE [MAP]\n"
+ " %s %s run PROG \\\n"
+ " data_in FILE \\\n"
+ " [data_out FILE [data_size_out L]] \\\n"
+ " [ctx_in FILE [ctx_out FILE [ctx_size_out M]]] \\\n"
+ " [repeat N]\n"
" %s %s tracelog\n"
" %s %s help\n"
"\n"
@@ -1079,7 +1423,8 @@ static int do_help(int argc, char **argv)
"",
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
- bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+ bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+ bin_name, argv[-2]);
return 0;
}
@@ -1095,6 +1440,7 @@ static const struct cmd cmds[] = {
{ "attach", do_attach },
{ "detach", do_detach },
{ "tracelog", do_tracelog },
+ { "run", do_run },
{ 0 }
};
diff --git a/tools/include/linux/sizes.h b/tools/include/linux/sizes.h
new file mode 100644
index 000000000000..1cbb4c4d016e
--- /dev/null
+++ b/tools/include/linux/sizes.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * include/linux/sizes.h
+ */
+#ifndef __LINUX_SIZES_H__
+#define __LINUX_SIZES_H__
+
+#include <linux/const.h>
+
+#define SZ_1 0x00000001
+#define SZ_2 0x00000002
+#define SZ_4 0x00000004
+#define SZ_8 0x00000008
+#define SZ_16 0x00000010
+#define SZ_32 0x00000020
+#define SZ_64 0x00000040
+#define SZ_128 0x00000080
+#define SZ_256 0x00000100
+#define SZ_512 0x00000200
+
+#define SZ_1K 0x00000400
+#define SZ_2K 0x00000800
+#define SZ_4K 0x00001000
+#define SZ_8K 0x00002000
+#define SZ_16K 0x00004000
+#define SZ_32K 0x00008000
+#define SZ_64K 0x00010000
+#define SZ_128K 0x00020000
+#define SZ_256K 0x00040000
+#define SZ_512K 0x00080000
+
+#define SZ_1M 0x00100000
+#define SZ_2M 0x00200000
+#define SZ_4M 0x00400000
+#define SZ_8M 0x00800000
+#define SZ_16M 0x01000000
+#define SZ_32M 0x02000000
+#define SZ_64M 0x04000000
+#define SZ_128M 0x08000000
+#define SZ_256M 0x10000000
+#define SZ_512M 0x20000000
+
+#define SZ_1G 0x40000000
+#define SZ_2G 0x80000000
+
+#define SZ_4G _AC(0x100000000, ULL)
+
+#endif /* __LINUX_SIZES_H__ */
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
From: Quentin Monnet @ 2019-07-05 17:50 UTC (permalink / raw)
To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
In-Reply-To: <CAH3MdRX3LLjcwi72tW_5TEj9sHvVqVE88xqX5Ud6MOZf83jUmw@mail.gmail.com>
2019-07-05 10:08 UTC-0700 ~ Y Song <ys114321@gmail.com>
> On Fri, Jul 5, 2019 at 9:03 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> 2019-07-05 08:42 UTC-0700 ~ Y Song <ys114321@gmail.com>
>>> On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
>>> <quentin.monnet@netronome.com> wrote:
>>>>
>>>> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
>>>>> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
>>>>> <quentin.monnet@netronome.com> wrote:
>>>>>>
>>>>>> Add a new "bpftool prog run" subcommand to run a loaded program on input
>>>>>> data (and possibly with input context) passed by the user.
>>>>>>
>>>>>> Print output data (and output context if relevant) into a file or into
>>>>>> the console. Print return value and duration for the test run into the
>>>>>> console.
>>>>>>
>>>>>> A "repeat" argument can be passed to run the program several times in a
>>>>>> row.
>>>>>>
>>>>>> The command does not perform any kind of verification based on program
>>>>>> type (Is this program type allowed to use an input context?) or on data
>>>>>> consistency (Can I work with empty input data?), this is left to the
>>>>>> kernel.
>>>>>>
>>>>>> Example invocation:
>>>>>>
>>>>>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
>>>>>> pinned /sys/fs/bpf/sample_ret0 \
>>>>>> data_in - data_out - repeat 5
>>>>>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
>>>>>> Return value: 0, duration (average): 260ns
>>>>>>
>>>>>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
>>>>>> in binary format. Other formats (JSON, hexdump) might be supported (via
>>>>>> an optional command line keyword like "data_fmt_in") in the future if
>>>>>> relevant, but this would require doing more parsing in bpftool.
>>>>>>
>>>>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>>>>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>>>>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
>>>>>> --- a/tools/bpf/bpftool/prog.c
>>>>>> +++ b/tools/bpf/bpftool/prog.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> #include <sys/stat.h>
>>>>>>
>>>>>> #include <linux/err.h>
>>>>>> +#include <linux/sizes.h>
>>>>>>
>>>>>> #include <bpf.h>
>>>>>> #include <btf.h>
>>>>>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static int check_single_stdin(char *file_in, char *other_file_in)
>>>>>> +{
>>>>>> + if (file_in && other_file_in &&
>>>>>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
>>>>>> + p_err("cannot use standard input for both data_in and ctx_in");
>>>>>
>>>>> The error message says data_in and ctx_in.
>>>>> Maybe the input parameter should be file_data_in and file_ctx_in?
>>>>
>>>>
>>>> Hi Yonghong,
>>>>
>>>> It's true those parameters should be file names. But having
>>>> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
>>>> command line seems a bit heavy to me? (And relying on keyword prefixing
>>>> for typing the command won't help much.)
>>>>
>>>> My opinion is that it should be clear from the man page or the "help"
>>>> command that the parameters are file names. What do you think? I can
>>>> prefix all four arguments with "file_" if you believe this is better.
>>>
>>> I think you misunderstood my question above.
>>
>> Totally did, sorry :/.
>>
>>> The command line parameters are fine.
>>> I am talking about the function parameter names. Since in the error message,
>>> the input parameters are referred for data_in and ctx_in
>>> p_err("cannot use standard input for both data_in and ctx_in")
>>> maybe the function signature should be
>>> static int check_single_stdin(char *file_data_in, char *file_ctx_in)
>>>
>>> If you are worried that later on the same function can be used in different
>>> contexts, then alternatively, you can have signature like
>>> static int check_single_stdin(char *file_in, char *other_file_in,
>>> const char *file_in_arg, const char *other_file_in_arg)
>>> where file_in_arg will be passed in as "data_in" and other_file_in_arg
>>> as "ctx_in".
>>> I think we could delay this until it is really needed.
>>
>> As a matter of fact, the opposite thing happened. I first used the
>> function for data_in/ctx_in, and also for data_out/ctx_out. But I
>> changed my mind eventually because there is no real reason not to print
>> both data_out and ctx_out to stdout if we want to do so. So I updated
>> the name of the parameters in the error messages, but forgot to change
>> the arguments for the function. Silly me.
>>
>> So I totally agree, I'll respin and change the argument names for the
>> function. And yes, we could also pass the names to print in the error
>> message, but I agree that this is not needed, and not helpful at the moment.
>>
>> Thanks for catching this!
>>
>>>>
>>>> [...]
>>>>
>>>>>> +static int do_run(int argc, char **argv)
>>>>>> +{
>>>>>> + char *data_fname_in = NULL, *data_fname_out = NULL;
>>>>>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
>>>>>> + struct bpf_prog_test_run_attr test_attr = {0};
>>>>>> + const unsigned int default_size = SZ_32K;
>>>>>> + void *data_in = NULL, *data_out = NULL;
>>>>>> + void *ctx_in = NULL, *ctx_out = NULL;
>>>>>> + unsigned int repeat = 1;
>>>>>> + int fd, err;
>>>>>> +
>>>>>> + if (!REQ_ARGS(4))
>>>>>> + return -1;
>>>>>> +
>>>>>> + fd = prog_parse_fd(&argc, &argv);
>>>>>> + if (fd < 0)
>>>>>> + return -1;
>>>>>> +
>>>>>> + while (argc) {
>>>>>> + if (detect_common_prefix(*argv, "data_in", "data_out",
>>>>>> + "data_size_out", NULL))
>>>>>> + return -1;
>>>>>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
>>>>>> + "ctx_size_out", NULL))
>>>>>> + return -1;
>>>>>> +
>>>>>> + if (is_prefix(*argv, "data_in")) {
>>>>>> + NEXT_ARG();
>>>>>> + if (!REQ_ARGS(1))
>>>>>> + return -1;
>>>>>> +
>>>>>> + data_fname_in = GET_ARG();
>>>>>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
>>>>>> + return -1;
>>>>>> + } else if (is_prefix(*argv, "data_out")) {
>>>>>
>>>>> Here, we all use is_prefix() to match "data_in", "data_out",
>>>>> "data_size_out" etc.
>>>>> That means users can use "data_i" instead of "data_in" as below
>>>>> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
>>>>> is this expected?
>>>> Yes, this is expected. We use prefix matching as we do pretty much
>>>> everywhere else in bpftool. It's not as useful here because most of the
>>>> strings for the names are similar. I agree that typing "data_i" instead
>>>> of "data_in" brings little advantage, but I see no reason why we should
>>>> reject prefixing for those keywords. And we accept "data_s" instead of
>>>> "data_size_out", which is still shorter to type than the complete keyword.
>>>
>>> This makes sense. Thanks for explanation.
>>>
>>> Another question. Currently, you are proposing "./bpftool prog run ...",
>>> but actually it is just a test_run. Do you think we should rename it
>>> to "./bpftool prog test_run ..." to make it clear for its intention?
>>
>> Good question. Hmm. It would make it more explicit that we use the
>> BPF_PROG_TEST_RUN command, but at the same time, from the point of view
>> of the user, there is nothing in particular that makes it a test run, is
>> it? I mean, you provide input data, you get output data and return
>> value, that makes it a real BPF run somehow, except that it's not on a
>> packet or anything. Do you think it is ambiguous and people may confuse
>> it with something like "attach"?
>
> I am more thinking about whether we could have a real "bpftool prog run ..."
> in the future which could really run the program in some kind of production
> environment...
>
> But I could be wrong since after "bpf prog attach" it may already just start
> to run in production, so "bpf prog run ..." not really needed for it.
>
> So "bpf prog run ..." is probably fine.
Ok, I'll stick to "prog run" unless someone else comments, then. I
suppose we can find something else, like "bpftool prog start", if we
need something like the feature you describe someday.
I'll send a v2 with the fix for the arguments in check_single_stdin().
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH rdma-next 0/2] DEVX VHCA tunnel support
From: Jason Gunthorpe @ 2019-07-05 17:40 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Max Gurtovoy,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190701181402.25286-1-leon@kernel.org>
On Mon, Jul 01, 2019 at 09:14:00PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Hi,
>
> Those two patches introduce VHCA tunnel mechanism to DEVX interface
> needed for Bluefield SOC. See extensive commit messages for more
> information.
>
> Thanks
>
> Max Gurtovoy (2):
> net/mlx5: Introduce VHCA tunnel device capability
> IB/mlx5: Implement VHCA tunnel mechanism in DEVX
>
> drivers/infiniband/hw/mlx5/devx.c | 24 ++++++++++++++++++++----
> include/linux/mlx5/mlx5_ifc.h | 10 ++++++++--
> 2 files changed, 28 insertions(+), 6 deletions(-)
This looks Ok can you apply the mlx5-next patch please
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH -next] carl9170: remove set but not used variable 'udev'
From: Christian Lamparter @ 2019-07-05 17:39 UTC (permalink / raw)
To: YueHaibing; +Cc: chunkeey, linux-kernel, netdev, linux-wireless, kvalo, davem
In-Reply-To: <20190702141207.47552-1-yuehaibing@huawei.com>
On Tuesday, July 2, 2019 4:12:07 PM CEST YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/ath/carl9170/usb.c: In function carl9170_usb_disconnect:
> drivers/net/wireless/ath/carl9170/usb.c:1110:21:
> warning: variable udev set but not used [-Wunused-but-set-variable]
>
> It is not use since commit feb09b293327 ("carl9170:
> fix misuse of device driver API")
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Acked-by: Christian Lamparter <chunkeey@gmail.com>
^ permalink raw reply
* Re: NEIGH: BUG, double timer add, state is 8
From: Lorenzo Bianconi @ 2019-07-05 17:30 UTC (permalink / raw)
To: David Ahern; +Cc: Marek Majkowski, David Miller, netdev, kernel-team
In-Reply-To: <1f4be489-4369-01d1-41c6-1406006df9c5@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]
On Jul 05, David Ahern wrote:
> On 7/4/19 3:59 PM, Marek Majkowski wrote:
> > I found a way to hit an obscure BUG in the
> > net/core/neighbour.c:neigh_add_timer(), by piping two carefully
> > crafted messages into AF_NETLINK socket.
> >
> > https://github.com/torvalds/linux/blob/v5.2-rc7/net/core/neighbour.c#L259
> >
> > if (unlikely(mod_timer(&n->timer, when))) {
> > printk("NEIGH: BUG, double timer add, state is %x\n", n->nud_state);
> > dump_stack();
> > }
> >
> > The repro is here:
> > https://gist.github.com/majek/d70297b9d72bc2e2b82145e122722a0c
> >
> > wget https://gist.githubusercontent.com/majek/d70297b9d72bc2e2b82145e122722a0c/raw/9e140bcedecc28d722022f1da142a379a9b7a7b0/double_timer_add_bug.c
>
> Thanks for the report - and the reproducer. I am on PTO through Monday;
> I will take a look next week if no one else does.
Hi David and Marek,
looking at the reproducer it seems to me the issue is due to the use of
'NTF_USE' from userspace.
Should we unschedule the neigh timer if we are in IN_TIMER receiving this
flag from userspace? (taking appropriate locking)
Regards,
Lorenzo
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: loss of connectivity after enabling vlan_filtering
From: Vivien Didelot @ 2019-07-05 17:29 UTC (permalink / raw)
To: vtolkm; +Cc: Andrew Lunn, netdev
In-Reply-To: <53bd8ffc-1c0a-334d-67d5-3a74b76670e8@gmail.com>
On Sun, 30 Jun 2019 01:23:02 +0200, vtolkm@googlemail.com wrote:
> A simple soul might infer that mv88e6xxx includes MV88E6060, at least
> that happened to me apparently (being said simpleton).
I agree that is confusing, that is why I don't like the 'xxx' naming
convention in general, found in many drivers. I'd prefer to stick with a
reference model, or product category, like soho in this case. But it was
initially written like this, so no reason to change its name now. I still
plan to merge mv88e6060 into mv88e6xxx, but it is unfortunately low priority
because I still don't have a platform with a 88E6060 on it.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCHv2] tools bpftool: Fix json dump crash on powerpc
From: Quentin Monnet @ 2019-07-05 17:26 UTC (permalink / raw)
To: Jakub Kicinski, Jiri Olsa
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Michael Petlan,
netdev, bpf, Martin KaFai Lau
In-Reply-To: <20190705102452.0831942a@cakuba.netronome.com>
2019-07-05 10:24 UTC-0700 ~ Jakub Kicinski <jakub.kicinski@netronome.com>
> On Fri, 5 Jul 2019 14:10:31 +0200, Jiri Olsa wrote:
>> Michael reported crash with by bpf program in json mode on powerpc:
>>
>> # bpftool prog -p dump jited id 14
>> [{
>> "name": "0xd00000000a9aa760",
>> "insns": [{
>> "pc": "0x0",
>> "operation": "nop",
>> "operands": [null
>> ]
>> },{
>> "pc": "0x4",
>> "operation": "nop",
>> "operands": [null
>> ]
>> },{
>> "pc": "0x8",
>> "operation": "mflr",
>> Segmentation fault (core dumped)
>>
>> The code is assuming char pointers in format, which is not always
>> true at least for powerpc. Fixing this by dumping the whole string
>> into buffer based on its format.
>>
>> Please note that libopcodes code does not check return values from
>> fprintf callback, but as per Jakub suggestion returning -1 on allocation
>> failure so we do the best effort to propagate the error.
>>
>> Reported-by: Michael Petlan <mpetlan@redhat.com>
>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Thanks, let me repost all the tags (Quentin, please shout if you're
> not ok with this :)):
I confirm it's all good for me, thanks :)
>
> Fixes: 107f041212c1 ("tools: bpftool: add JSON output for `bpftool prog dump jited *` command")
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
^ permalink raw reply
* Re: [PATCHv2] tools bpftool: Fix json dump crash on powerpc
From: Jakub Kicinski @ 2019-07-05 17:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Michael Petlan,
netdev, bpf, Martin KaFai Lau, Quentin Monnet
In-Reply-To: <20190705121031.GA10777@krava>
On Fri, 5 Jul 2019 14:10:31 +0200, Jiri Olsa wrote:
> Michael reported crash with by bpf program in json mode on powerpc:
>
> # bpftool prog -p dump jited id 14
> [{
> "name": "0xd00000000a9aa760",
> "insns": [{
> "pc": "0x0",
> "operation": "nop",
> "operands": [null
> ]
> },{
> "pc": "0x4",
> "operation": "nop",
> "operands": [null
> ]
> },{
> "pc": "0x8",
> "operation": "mflr",
> Segmentation fault (core dumped)
>
> The code is assuming char pointers in format, which is not always
> true at least for powerpc. Fixing this by dumping the whole string
> into buffer based on its format.
>
> Please note that libopcodes code does not check return values from
> fprintf callback, but as per Jakub suggestion returning -1 on allocation
> failure so we do the best effort to propagate the error.
>
> Reported-by: Michael Petlan <mpetlan@redhat.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Thanks, let me repost all the tags (Quentin, please shout if you're
not ok with this :)):
Fixes: 107f041212c1 ("tools: bpftool: add JSON output for `bpftool prog dump jited *` command")
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: [rdma 14/16] RDMA/irdma: Add ABI definitions
From: Jason Gunthorpe @ 2019-07-05 17:16 UTC (permalink / raw)
To: Saleem, Shiraz
Cc: Leon Romanovsky, Kirsher, Jeffrey T, dledford@redhat.com,
davem@davemloft.net, Ismail, Mustafa, linux-rdma@vger.kernel.org,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
poswald@suse.com, Ertman, David M
In-Reply-To: <9DD61F30A802C4429A01CA4200E302A7A684DAAA@fmsmsx124.amr.corp.intel.com>
On Fri, Jul 05, 2019 at 04:42:19PM +0000, Saleem, Shiraz wrote:
> > Subject: Re: [rdma 14/16] RDMA/irdma: Add ABI definitions
> >
> > On Thu, Jul 04, 2019 at 10:40:21AM +0300, Leon Romanovsky wrote:
> > > On Wed, Jul 03, 2019 at 07:12:57PM -0700, Jeff Kirsher wrote:
> > > > From: Mustafa Ismail <mustafa.ismail@intel.com>
> > > >
> > > > Add ABI definitions for irdma.
> > > >
> > > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> > > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > > > include/uapi/rdma/irdma-abi.h | 130
> > > > ++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 130 insertions(+)
> > > > create mode 100644 include/uapi/rdma/irdma-abi.h
> > > >
> > > > diff --git a/include/uapi/rdma/irdma-abi.h
> > > > b/include/uapi/rdma/irdma-abi.h new file mode 100644 index
> > > > 000000000000..bdfbda4c829e
> > > > +++ b/include/uapi/rdma/irdma-abi.h
> > > > @@ -0,0 +1,130 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> > > > +/* Copyright (c) 2006 - 2019 Intel Corporation. All rights reserved.
> > > > + * Copyright (c) 2005 Topspin Communications. All rights reserved.
> > > > + * Copyright (c) 2005 Cisco Systems. All rights reserved.
> > > > + * Copyright (c) 2005 Open Grid Computing, Inc. All rights reserved.
> > > > + */
> > > > +
> > > > +#ifndef IRDMA_ABI_H
> > > > +#define IRDMA_ABI_H
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +/* irdma must support legacy GEN_1 i40iw kernel
> > > > + * and user-space whose last ABI ver is 5 */ #define IRDMA_ABI_VER
> > > > +6
> > >
> > > Can you please elaborate about it more?
> > > There is no irdma code in RDMA yet, so it makes me wonder why new
> > > define shouldn't start from 1.
> >
> > It is because they are ABI compatible with the current user space, which raises the
> > question why we even have this confusing header file..
>
> It is because we need to support current providers/i40iw user-space.
> Our user-space patch series will introduce a new provider (irdma) whose ABI
> ver. is also 6 (capable of supporting X722 and which will work with i40iw driver
> on older kernels) and removes providers/i40iw from rdma-core.
Why on earth would we do that?
Jason
^ permalink raw reply
* Re: [PATCH net-next 8/8] net: mscc: PTP Hardware Clock (PHC) support
From: Antoine Tenart @ 2019-07-05 17:16 UTC (permalink / raw)
To: Richard Cochran
Cc: Antoine Tenart, davem, alexandre.belloni, UNGLinuxDriver, ralf,
paul.burton, jhogan, netdev, linux-mips, thomas.petazzoni,
allan.nielsen
In-Reply-To: <20190705164736.x6dy2oc6jo5db65v@localhost>
Hello Richard,
On Fri, Jul 05, 2019 at 09:47:36AM -0700, Richard Cochran wrote:
> On Mon, Jul 01, 2019 at 12:03:27PM +0200, Antoine Tenart wrote:
>
> > +void ocelot_get_hwtimestamp(struct ocelot *ocelot, struct timespec64 *ts)
> > +{
> > + /* Read current PTP time to get seconds */
> > + u32 val = ocelot_read_rix(ocelot, PTP_PIN_CFG, TOD_ACC_PIN);
>
> This register is protected by ocelot->ptp_clock_lock from other code
> paths, but not in this one!
Oops. I'll fix it.
> > +static int ocelot_init_timestamp(struct ocelot *ocelot)
> > +{
> > + ocelot->ptp_info = ocelot_ptp_clock_info;
> > +
> > + ocelot->ptp_clock = ptp_clock_register(&ocelot->ptp_info, ocelot->dev);
> > + if (IS_ERR(ocelot->ptp_clock))
> > + return PTR_ERR(ocelot->ptp_clock);
>
> You need to handle the NULL case:
Will do.
> ptp_clock_register() - register a PTP hardware clock driver
>
> @info: Structure describing the new clock.
> @parent: Pointer to the parent device of the new clock.
>
> Returns a valid pointer on success or PTR_ERR on failure. If PHC
> support is missing at the configuration level, this function
> returns NULL, and drivers are expected to gracefully handle that
> case separately.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH bpf-next] tools: bpftool: add "prog run" subcommand to test-run programs
From: Y Song @ 2019-07-05 17:08 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers
In-Reply-To: <4e7a66b8-8c4b-58cc-61a8-9ec6568d4df7@netronome.com>
On Fri, Jul 5, 2019 at 9:03 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> 2019-07-05 08:42 UTC-0700 ~ Y Song <ys114321@gmail.com>
> > On Fri, Jul 5, 2019 at 1:21 AM Quentin Monnet
> > <quentin.monnet@netronome.com> wrote:
> >>
> >> 2019-07-04 22:49 UTC-0700 ~ Y Song <ys114321@gmail.com>
> >>> On Thu, Jul 4, 2019 at 1:58 AM Quentin Monnet
> >>> <quentin.monnet@netronome.com> wrote:
> >>>>
> >>>> Add a new "bpftool prog run" subcommand to run a loaded program on input
> >>>> data (and possibly with input context) passed by the user.
> >>>>
> >>>> Print output data (and output context if relevant) into a file or into
> >>>> the console. Print return value and duration for the test run into the
> >>>> console.
> >>>>
> >>>> A "repeat" argument can be passed to run the program several times in a
> >>>> row.
> >>>>
> >>>> The command does not perform any kind of verification based on program
> >>>> type (Is this program type allowed to use an input context?) or on data
> >>>> consistency (Can I work with empty input data?), this is left to the
> >>>> kernel.
> >>>>
> >>>> Example invocation:
> >>>>
> >>>> # perl -e 'print "\x0" x 14' | ./bpftool prog run \
> >>>> pinned /sys/fs/bpf/sample_ret0 \
> >>>> data_in - data_out - repeat 5
> >>>> 0000000 0000 0000 0000 0000 0000 0000 0000 | ........ ......
> >>>> Return value: 0, duration (average): 260ns
> >>>>
> >>>> When one of data_in or ctx_in is "-", bpftool reads from standard input,
> >>>> in binary format. Other formats (JSON, hexdump) might be supported (via
> >>>> an optional command line keyword like "data_fmt_in") in the future if
> >>>> relevant, but this would require doing more parsing in bpftool.
> >>>>
> >>>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> >>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> >>>> ---
> >>
> >> [...]
> >>
> >>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> >>>> index 9b0db5d14e31..8dcbaa0a8ab1 100644
> >>>> --- a/tools/bpf/bpftool/prog.c
> >>>> +++ b/tools/bpf/bpftool/prog.c
> >>>> @@ -15,6 +15,7 @@
> >>>> #include <sys/stat.h>
> >>>>
> >>>> #include <linux/err.h>
> >>>> +#include <linux/sizes.h>
> >>>>
> >>>> #include <bpf.h>
> >>>> #include <btf.h>
> >>>> @@ -748,6 +749,344 @@ static int do_detach(int argc, char **argv)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static int check_single_stdin(char *file_in, char *other_file_in)
> >>>> +{
> >>>> + if (file_in && other_file_in &&
> >>>> + !strcmp(file_in, "-") && !strcmp(other_file_in, "-")) {
> >>>> + p_err("cannot use standard input for both data_in and ctx_in");
> >>>
> >>> The error message says data_in and ctx_in.
> >>> Maybe the input parameter should be file_data_in and file_ctx_in?
> >>
> >>
> >> Hi Yonghong,
> >>
> >> It's true those parameters should be file names. But having
> >> "file_data_in", "file_data_out", "file_ctx_in" and "file_ctx_out" on a
> >> command line seems a bit heavy to me? (And relying on keyword prefixing
> >> for typing the command won't help much.)
> >>
> >> My opinion is that it should be clear from the man page or the "help"
> >> command that the parameters are file names. What do you think? I can
> >> prefix all four arguments with "file_" if you believe this is better.
> >
> > I think you misunderstood my question above.
>
> Totally did, sorry :/.
>
> > The command line parameters are fine.
> > I am talking about the function parameter names. Since in the error message,
> > the input parameters are referred for data_in and ctx_in
> > p_err("cannot use standard input for both data_in and ctx_in")
> > maybe the function signature should be
> > static int check_single_stdin(char *file_data_in, char *file_ctx_in)
> >
> > If you are worried that later on the same function can be used in different
> > contexts, then alternatively, you can have signature like
> > static int check_single_stdin(char *file_in, char *other_file_in,
> > const char *file_in_arg, const char *other_file_in_arg)
> > where file_in_arg will be passed in as "data_in" and other_file_in_arg
> > as "ctx_in".
> > I think we could delay this until it is really needed.
>
> As a matter of fact, the opposite thing happened. I first used the
> function for data_in/ctx_in, and also for data_out/ctx_out. But I
> changed my mind eventually because there is no real reason not to print
> both data_out and ctx_out to stdout if we want to do so. So I updated
> the name of the parameters in the error messages, but forgot to change
> the arguments for the function. Silly me.
>
> So I totally agree, I'll respin and change the argument names for the
> function. And yes, we could also pass the names to print in the error
> message, but I agree that this is not needed, and not helpful at the moment.
>
> Thanks for catching this!
>
> >>
> >> [...]
> >>
> >>>> +static int do_run(int argc, char **argv)
> >>>> +{
> >>>> + char *data_fname_in = NULL, *data_fname_out = NULL;
> >>>> + char *ctx_fname_in = NULL, *ctx_fname_out = NULL;
> >>>> + struct bpf_prog_test_run_attr test_attr = {0};
> >>>> + const unsigned int default_size = SZ_32K;
> >>>> + void *data_in = NULL, *data_out = NULL;
> >>>> + void *ctx_in = NULL, *ctx_out = NULL;
> >>>> + unsigned int repeat = 1;
> >>>> + int fd, err;
> >>>> +
> >>>> + if (!REQ_ARGS(4))
> >>>> + return -1;
> >>>> +
> >>>> + fd = prog_parse_fd(&argc, &argv);
> >>>> + if (fd < 0)
> >>>> + return -1;
> >>>> +
> >>>> + while (argc) {
> >>>> + if (detect_common_prefix(*argv, "data_in", "data_out",
> >>>> + "data_size_out", NULL))
> >>>> + return -1;
> >>>> + if (detect_common_prefix(*argv, "ctx_in", "ctx_out",
> >>>> + "ctx_size_out", NULL))
> >>>> + return -1;
> >>>> +
> >>>> + if (is_prefix(*argv, "data_in")) {
> >>>> + NEXT_ARG();
> >>>> + if (!REQ_ARGS(1))
> >>>> + return -1;
> >>>> +
> >>>> + data_fname_in = GET_ARG();
> >>>> + if (check_single_stdin(data_fname_in, ctx_fname_in))
> >>>> + return -1;
> >>>> + } else if (is_prefix(*argv, "data_out")) {
> >>>
> >>> Here, we all use is_prefix() to match "data_in", "data_out",
> >>> "data_size_out" etc.
> >>> That means users can use "data_i" instead of "data_in" as below
> >>> ... | ./bpftool prog run id 283 data_i - data_out - repeat 5
> >>> is this expected?
> >> Yes, this is expected. We use prefix matching as we do pretty much
> >> everywhere else in bpftool. It's not as useful here because most of the
> >> strings for the names are similar. I agree that typing "data_i" instead
> >> of "data_in" brings little advantage, but I see no reason why we should
> >> reject prefixing for those keywords. And we accept "data_s" instead of
> >> "data_size_out", which is still shorter to type than the complete keyword.
> >
> > This makes sense. Thanks for explanation.
> >
> > Another question. Currently, you are proposing "./bpftool prog run ...",
> > but actually it is just a test_run. Do you think we should rename it
> > to "./bpftool prog test_run ..." to make it clear for its intention?
>
> Good question. Hmm. It would make it more explicit that we use the
> BPF_PROG_TEST_RUN command, but at the same time, from the point of view
> of the user, there is nothing in particular that makes it a test run, is
> it? I mean, you provide input data, you get output data and return
> value, that makes it a real BPF run somehow, except that it's not on a
> packet or anything. Do you think it is ambiguous and people may confuse
> it with something like "attach"?
I am more thinking about whether we could have a real "bpftool prog run ..."
in the future which could really run the program in some kind of production
environment...
But I could be wrong since after "bpf prog attach" it may already just start
to run in production, so "bpf prog run ..." not really needed for it.
So "bpf prog run ..." is probably fine.
>
> Thanks,
> Quentin
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: net: Add bindings for Realtek PHYs
From: Rob Herring @ 2019-07-05 17:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: Matthias Kaehlcke, David S . Miller, Mark Rutland,
Florian Fainelli, Heiner Kallweit, netdev, devicetree,
linux-kernel@vger.kernel.org, Douglas Anderson
In-Reply-To: <20190705162926.GM18473@lunn.ch>
On Fri, Jul 5, 2019 at 10:29 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Jul 05, 2019 at 10:17:16AM -0600, Rob Herring wrote:
> > On Wed, Jul 3, 2019 at 3:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > I think if we're going to have custom properties for phys, we should
> > > > have a compatible string to at least validate whether the custom
> > > > properties are even valid for the node.
> > >
> > > Hi Rob
> > >
> > > What happens with other enumerable busses where a compatible string is
> > > not used?
> >
> > We usually have a compatible. USB and PCI both do. Sometimes it is a
> > defined format based on VID/PID.
>
> Hi Rob
>
> Is it defined what to do with this compatible? Just totally ignore it?
> Validate it against the hardware and warning if it is wrong? Force
> load the driver that implements the compatible, even thought bus
> enumeration says it is the wrong driver?
The short answer is either the problems get fixed or if DTs exist and
need to be supported which are wrong then the OS deals with the
problem to make things work as desired (see PowerMac code).
If the ethernet phy subsystem wants to ignore compatible, that is totally fine.
> > > The Ethernet PHY subsystem will ignore the compatible string and load
> > > the driver which fits the enumeration data. Using the compatible
> > > string only to get the right YAML validator seems wrong. I would
> > > prefer adding some other property with a clear name indicates its is
> > > selecting the validator, and has nothing to do with loading the
> > > correct driver. And it can then be used as well for USB and PCI
> > > devices etc.
> >
> > Just because Linux happens to not use compatible really has nothing to
> > do with whether or not the nodes should have a compatible. What does
> > FreeBSD want? U-boot?
> >
> > I don't follow how adding a validate property would help. It would
> > need to be 'validate-node-as-a-realtek-phy'.
>
> This makes it clear it is all about validating the DT, and nothing
> about the actual running hardware. What i don't really want to see is
> the poorly defined situation that DT contains a compatible string, but
> we have no idea what it is actually used for. See the question above.
What's poorly defined are the current bindings, type definitions of
properties, and what properties are valid or not in specific nodes. If
we only had to better define the rules around compatible use or
mismatches, we'd be a lot better off.
I'm not going to add properties solely for validation when we already
have a well defined, 15 year+ pattern for defining what a node
contains that practically every other subsystem and node uses. I guess
we just won't worry about validating ethernet phy nodes beyond some
basic checks.
Rob
^ permalink raw reply
* Re: [PATCH net-next 8/8] net: mscc: PTP Hardware Clock (PHC) support
From: Richard Cochran @ 2019-07-05 16:47 UTC (permalink / raw)
To: Antoine Tenart
Cc: davem, alexandre.belloni, UNGLinuxDriver, ralf, paul.burton,
jhogan, netdev, linux-mips, thomas.petazzoni, allan.nielsen
In-Reply-To: <20190701100327.6425-9-antoine.tenart@bootlin.com>
On Mon, Jul 01, 2019 at 12:03:27PM +0200, Antoine Tenart wrote:
> +void ocelot_get_hwtimestamp(struct ocelot *ocelot, struct timespec64 *ts)
> +{
> + /* Read current PTP time to get seconds */
> + u32 val = ocelot_read_rix(ocelot, PTP_PIN_CFG, TOD_ACC_PIN);
This register is protected by ocelot->ptp_clock_lock from other code
paths, but not in this one!
> + val &= ~(PTP_PIN_CFG_SYNC | PTP_PIN_CFG_ACTION_MASK | PTP_PIN_CFG_DOM);
> + val |= PTP_PIN_CFG_ACTION(PTP_PIN_ACTION_SAVE);
> + ocelot_write_rix(ocelot, val, PTP_PIN_CFG, TOD_ACC_PIN);
> + ts->tv_sec = ocelot_read_rix(ocelot, PTP_PIN_TOD_SEC_LSB, TOD_ACC_PIN);
...
> +}
> +static int ocelot_init_timestamp(struct ocelot *ocelot)
> +{
> + ocelot->ptp_info = ocelot_ptp_clock_info;
> +
> + ocelot->ptp_clock = ptp_clock_register(&ocelot->ptp_info, ocelot->dev);
> + if (IS_ERR(ocelot->ptp_clock))
> + return PTR_ERR(ocelot->ptp_clock);
You need to handle the NULL case:
ptp_clock_register() - register a PTP hardware clock driver
@info: Structure describing the new clock.
@parent: Pointer to the parent device of the new clock.
Returns a valid pointer on success or PTR_ERR on failure. If PHC
support is missing at the configuration level, this function
returns NULL, and drivers are expected to gracefully handle that
case separately.
> +
> + ocelot_write(ocelot, SYS_PTP_CFG_PTP_STAMP_WID(30), SYS_PTP_CFG);
> + ocelot_write(ocelot, 0xffffffff, ANA_TABLES_PTP_ID_LOW);
> + ocelot_write(ocelot, 0xffffffff, ANA_TABLES_PTP_ID_HIGH);
> +
> + ocelot_write(ocelot, PTP_CFG_MISC_PTP_EN, PTP_CFG_MISC);
> +
> + return 0;
> +}
Thanks,
Richard
^ permalink raw reply
* RE: [rdma 14/16] RDMA/irdma: Add ABI definitions
From: Saleem, Shiraz @ 2019-07-05 16:42 UTC (permalink / raw)
To: Jason Gunthorpe, Leon Romanovsky
Cc: Kirsher, Jeffrey T, dledford@redhat.com, davem@davemloft.net,
Ismail, Mustafa, linux-rdma@vger.kernel.org,
netdev@vger.kernel.org, nhorman@redhat.com, sassmann@redhat.com,
poswald@suse.com, Ertman, David M
In-Reply-To: <20190704121933.GD3401@mellanox.com>
> Subject: Re: [rdma 14/16] RDMA/irdma: Add ABI definitions
>
> On Thu, Jul 04, 2019 at 10:40:21AM +0300, Leon Romanovsky wrote:
> > On Wed, Jul 03, 2019 at 07:12:57PM -0700, Jeff Kirsher wrote:
> > > From: Mustafa Ismail <mustafa.ismail@intel.com>
> > >
> > > Add ABI definitions for irdma.
> > >
> > > Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
> > > Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
> > > include/uapi/rdma/irdma-abi.h | 130
> > > ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 130 insertions(+)
> > > create mode 100644 include/uapi/rdma/irdma-abi.h
> > >
> > > diff --git a/include/uapi/rdma/irdma-abi.h
> > > b/include/uapi/rdma/irdma-abi.h new file mode 100644 index
> > > 000000000000..bdfbda4c829e
> > > +++ b/include/uapi/rdma/irdma-abi.h
> > > @@ -0,0 +1,130 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> > > +/* Copyright (c) 2006 - 2019 Intel Corporation. All rights reserved.
> > > + * Copyright (c) 2005 Topspin Communications. All rights reserved.
> > > + * Copyright (c) 2005 Cisco Systems. All rights reserved.
> > > + * Copyright (c) 2005 Open Grid Computing, Inc. All rights reserved.
> > > + */
> > > +
> > > +#ifndef IRDMA_ABI_H
> > > +#define IRDMA_ABI_H
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/* irdma must support legacy GEN_1 i40iw kernel
> > > + * and user-space whose last ABI ver is 5 */ #define IRDMA_ABI_VER
> > > +6
> >
> > Can you please elaborate about it more?
> > There is no irdma code in RDMA yet, so it makes me wonder why new
> > define shouldn't start from 1.
>
> It is because they are ABI compatible with the current user space, which raises the
> question why we even have this confusing header file..
It is because we need to support current providers/i40iw user-space.
Our user-space patch series will introduce a new provider (irdma) whose ABI
ver. is also 6 (capable of supporting X722 and which will work with i40iw driver
on older kernels) and removes providers/i40iw from rdma-core.
^ permalink raw reply
* Re: [PATCH net-next 1/8] Documentation/bindings: net: ocelot: document the PTP bank
From: Antoine Tenart @ 2019-07-05 16:39 UTC (permalink / raw)
To: Andrew Lunn
Cc: Antoine Tenart, davem, richardcochran, alexandre.belloni,
UNGLinuxDriver, ralf, paul.burton, jhogan, netdev, linux-mips,
thomas.petazzoni, allan.nielsen
In-Reply-To: <20190705144517.GD4428@lunn.ch>
Hi Andrew,
On Fri, Jul 05, 2019 at 04:45:17PM +0200, Andrew Lunn wrote:
> On Fri, Jul 05, 2019 at 03:30:16PM +0200, Antoine Tenart wrote:
> >
> > I'm not sure about this: optional properties means some parts of the h/w
> > can be missing or not wired. It's not the case here, it's "optional" in
> > the driver only for dt compatibility (so that an older dt blob can work
> > with a newer kernel image), but it's now mandatory in the binding.
>
> If the driver can work without it, it is clearly optional. You just
> get reduced functionality. That is the thing with DT. You can never
> add more required properties after the first commit without breaking
> backwards compatibility. To make the documentation fit the driver,
> somewhere you need to state they are optional. Either by placing the
> new properties in the optional section of the binding, or add a
> comment.
The documentation is unrelated to the driver. It's the documentation of
the binding itself, which is only describing the h/w.
But I discussed this with a someone and I got to the same conclusion as
your statement, because there can be old dt blobs in the wild and the
binding documentation can be used to make new code. That code should be
aware of required/optional properties.
I'll fix this in v2.
Thanks!
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* RE: [net-next 1/3] ice: Initialize and register platform device to provide RDMA
From: Saleem, Shiraz @ 2019-07-05 16:33 UTC (permalink / raw)
To: Greg KH, Jason Gunthorpe
Cc: Kirsher, Jeffrey T, davem@davemloft.net, dledford@redhat.com,
Nguyen, Anthony L, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, nhorman@redhat.com,
sassmann@redhat.com, poswald@suse.com, Ismail, Mustafa,
Ertman, David M, Bowers, AndrewX
In-Reply-To: <20190704134612.GB10963@kroah.com>
> Subject: Re: [net-next 1/3] ice: Initialize and register platform device to provide
> RDMA
>
> On Thu, Jul 04, 2019 at 12:48:29PM +0000, Jason Gunthorpe wrote:
> > On Thu, Jul 04, 2019 at 02:42:47PM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 12:37:33PM +0000, Jason Gunthorpe wrote:
> > > > On Thu, Jul 04, 2019 at 02:29:50PM +0200, Greg KH wrote:
> > > > > On Thu, Jul 04, 2019 at 12:16:41PM +0000, Jason Gunthorpe wrote:
> > > > > > On Wed, Jul 03, 2019 at 07:12:50PM -0700, Jeff Kirsher wrote:
> > > > > > > From: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > > > > >
> > > > > > > The RDMA block does not advertise on the PCI bus or any other bus.
> > > > > > > Thus the ice driver needs to provide access to the RDMA
> > > > > > > hardware block via a virtual bus; utilize the platform bus to provide this
> access.
> > > > > > >
> > > > > > > This patch initializes the driver to support RDMA as well as
> > > > > > > creates and registers a platform device for the RDMA driver
> > > > > > > to register to. At this point the driver is fully
> > > > > > > initialized to register a platform driver, however, can not
> > > > > > > yet register as the ops have not been implemented.
> > > > > >
> > > > > > I think you need Greg's ack on all this driver stuff -
> > > > > > particularly that a platform_device is OK.
> > > > >
> > > > > A platform_device is almost NEVER ok.
> > > > >
> > > > > Don't abuse it, make a real device on a real bus. If you don't
> > > > > have a real bus and just need to create a device to hang other
> > > > > things off of, then use the virtual one, that's what it is there for.
> > > >
> > > > Ideally I'd like to see all the RDMA drivers that connect to
> > > > ethernet drivers use some similar scheme.
> > >
> > > Why? They should be attached to a "real" device, why make any up?
> >
> > ? A "real" device, like struct pci_device, can only bind to one
> > driver. How can we bind it concurrently to net, rdma, scsi, etc?
>
> MFD was designed for this very problem.
>
> > > > This is for a PCI device that plugs into multiple subsystems in
> > > > the kernel, ie it has net driver functionality, rdma
> > > > functionality, some even have SCSI functionality
> > >
> > > Sounds like a MFD device, why aren't you using that functionality
> > > instead?
> >
> > This was also my advice, but in another email Jeff says:
> >
> > MFD architecture was also considered, and we selected the simpler
> > platform model. Supporting a MFD architecture would require an
> > additional MFD core driver, individual platform netdev, RDMA function
> > drivers, and stripping a large portion of the netdev drivers into
> > MFD core. The sub-devices registered by MFD core for function
> > drivers are indeed platform devices.
>
> So, "mfd is too hard, let's abuse a platform device" is ok?
>
> People have been wanting to do MFD drivers for PCI devices for a long time, it's
> about time someone actually did the work for it, I bet it will not be all that complex
> if tiny embedded drivers can do it :)
>
Hi Greg - Thanks for your feedback!
We currently have 2 PCI function netdev drivers in the kernel (i40e & ice) that support devices (x722 & e810)
which are RDMA capable. Our objective is to add a single unified RDMA driver
(as this a subsystem specific requirement) which needs to access HW resources from the
netdev PF drivers. Attaching platform devices from the netdev drivers to the platform bus
and having a single RDMA platform driver bind to them and access these resources seemed
like a simple approach to realize our objective. But seems like attaching platform devices is
wrong. I would like to understand why.
Are platform sub devices only to be added from an MFD core driver? I am also wondering if MFD arch.
would allow for realizing a single RDMA driver and whether we need an MFD core driver for
each device, x722 & e810 or whether it can be a single driver.
Shiraz
^ permalink raw reply
* Re: [PATCH v2 1/7] dt-bindings: net: Add bindings for Realtek PHYs
From: Andrew Lunn @ 2019-07-05 16:29 UTC (permalink / raw)
To: Rob Herring
Cc: Matthias Kaehlcke, David S . Miller, Mark Rutland,
Florian Fainelli, Heiner Kallweit, netdev, devicetree,
linux-kernel@vger.kernel.org, Douglas Anderson
In-Reply-To: <CAL_Jsq+dqz7n0_+Y5R4772-rh=9x=k20A69hnDwxH3OyZXQneQ@mail.gmail.com>
On Fri, Jul 05, 2019 at 10:17:16AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 3:33 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > I think if we're going to have custom properties for phys, we should
> > > have a compatible string to at least validate whether the custom
> > > properties are even valid for the node.
> >
> > Hi Rob
> >
> > What happens with other enumerable busses where a compatible string is
> > not used?
>
> We usually have a compatible. USB and PCI both do. Sometimes it is a
> defined format based on VID/PID.
Hi Rob
Is it defined what to do with this compatible? Just totally ignore it?
Validate it against the hardware and warning if it is wrong? Force
load the driver that implements the compatible, even thought bus
enumeration says it is the wrong driver?
> > The Ethernet PHY subsystem will ignore the compatible string and load
> > the driver which fits the enumeration data. Using the compatible
> > string only to get the right YAML validator seems wrong. I would
> > prefer adding some other property with a clear name indicates its is
> > selecting the validator, and has nothing to do with loading the
> > correct driver. And it can then be used as well for USB and PCI
> > devices etc.
>
> Just because Linux happens to not use compatible really has nothing to
> do with whether or not the nodes should have a compatible. What does
> FreeBSD want? U-boot?
>
> I don't follow how adding a validate property would help. It would
> need to be 'validate-node-as-a-realtek-phy'.
This makes it clear it is all about validating the DT, and nothing
about the actual running hardware. What i don't really want to see is
the poorly defined situation that DT contains a compatible string, but
we have no idea what it is actually used for. See the question above.
Andrew
^ permalink raw reply
* Re: [PATCH bpf-next RFC v3 2/6] bpf: add BPF_MAP_DUMP command to dump more than one entry per call
From: Y Song @ 2019-07-05 16:22 UTC (permalink / raw)
To: Brian Vazquez
Cc: Brian Vazquez, Alexei Starovoitov, Daniel Borkmann,
David S . Miller, Stanislav Fomichev, Willem de Bruijn,
Petar Penkov, LKML, netdev, bpf
In-Reply-To: <20190703170118.196552-3-brianvv@google.com>
On Wed, Jul 3, 2019 at 10:03 AM Brian Vazquez <brianvv@google.com> wrote:
>
> This introduces a new command to retrieve a variable number of entries
> from a bpf map wrapping the existing bpf methods:
> map_get_next_key and map_lookup_elem
>
> To start dumping the map from the beginning you must specify NULL as
> the prev_key.
>
> The new API returns 0 when it successfully copied all the elements
> requested or it copied less because there weren't more elements to
> retrieved (err == -ENOENT). In last scenario err will be masked to 0.
>
> On a successful call buf and buf_len will contain correct data and in
> case prev_key was provided (not for the first walk, since prev_key is
> NULL) it will contain the last_key copied into the buf which will simplify
> next call.
>
> Only when it can't find a single element it will return -ENOENT meaning
> that the map has been entirely walked. When an error is return buf,
> buf_len and prev_key shouldn't be read nor used.
>
> Because maps can be called from userspace and kernel code, this function
> can have a scenario where the next_key was found but by the time we
> try to retrieve the value the element is not there, in this case the
> function continues and tries to get a new next_key value, skipping the
> deleted key. If at some point the function find itself trap in a loop,
> it will return -EINTR.
>
> The function will try to fit as much as possible in the buf provided and
> will return -EINVAL if buf_len is smaller than elem_size.
>
> QUEUE and STACK maps are not supported.
>
> Note that map_dump doesn't guarantee that reading the entire table is
> consistent since this function is always racing with kernel and user code
> but the same behaviour is found when the entire table is walked using
> the current interfaces: map_get_next_key + map_lookup_elem.
> It is also important to note that when a locked map the lock is grabbed for
> 1 entry at the time, meaning that the buf returned might or might not be
> consistent.
First, thanks for the RFC. I do think there are use cases where
batch dumping helps.
Some comments below.
>
> Suggested-by: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Brian Vazquez <brianvv@google.com>
> ---
> include/uapi/linux/bpf.h | 9 +++
> kernel/bpf/syscall.c | 118 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 127 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index cffea1826a1f..cc589570a639 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -106,6 +106,7 @@ enum bpf_cmd {
> BPF_TASK_FD_QUERY,
> BPF_MAP_LOOKUP_AND_DELETE_ELEM,
> BPF_MAP_FREEZE,
> + BPF_MAP_DUMP,
> };
>
> enum bpf_map_type {
> @@ -388,6 +389,14 @@ union bpf_attr {
> __u64 flags;
> };
>
> + struct { /* struct used by BPF_MAP_DUMP command */
> + __u32 map_fd;
> + __aligned_u64 prev_key;
> + __aligned_u64 buf;
> + __aligned_u64 buf_len; /* input/output: len of buf */
> + __u64 flags;
> + } dump;
Maybe you can swap map_fd and flags?
This way, you won't have hole right after map_fd?
> +
> struct { /* anonymous struct used by BPF_PROG_LOAD command */
> __u32 prog_type; /* one of enum bpf_prog_type */
> __u32 insn_cnt;
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d200d2837ade..78d55463fc76 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1097,6 +1097,121 @@ static int map_get_next_key(union bpf_attr *attr)
> return err;
> }
>
> +/* last field in 'union bpf_attr' used by this command */
> +#define BPF_MAP_DUMP_LAST_FIELD dump.buf_len
> +
> +static int map_dump(union bpf_attr *attr)
> +{
> + void __user *ukey = u64_to_user_ptr(attr->dump.prev_key);
> + void __user *ubuf = u64_to_user_ptr(attr->dump.buf);
> + u32 __user *ubuf_len = u64_to_user_ptr(attr->dump.buf_len);
> + int ufd = attr->dump.map_fd;
> + struct bpf_map *map;
> + void *buf, *prev_key, *key, *value;
> + u32 value_size, elem_size, buf_len, cp_len;
> + struct fd f;
> + int err;
> + bool first_key = false;
> +
> + if (CHECK_ATTR(BPF_MAP_DUMP))
> + return -EINVAL;
> +
> + attr->flags = 0;
Why do you want attr->flags? This is to modify anonumous struct used by
BPF_MAP_*_ELEM commands.
> + if (attr->dump.flags & ~BPF_F_LOCK)
> + return -EINVAL;
> +
> + f = fdget(ufd);
> + map = __bpf_map_get(f);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> + if (!(map_get_sys_perms(map, f) & FMODE_CAN_READ)) {
> + err = -EPERM;
> + goto err_put;
> + }
> +
> + if ((attr->dump.flags & BPF_F_LOCK) &&
> + !map_value_has_spin_lock(map)) {
> + err = -EINVAL;
> + goto err_put;
> + }
> +
> + if (map->map_type == BPF_MAP_TYPE_QUEUE ||
> + map->map_type == BPF_MAP_TYPE_STACK) {
> + err = -ENOTSUPP;
> + goto err_put;
> + }
> +
> + value_size = bpf_map_value_size(map);
> +
> + err = get_user(buf_len, ubuf_len);
> + if (err)
> + goto err_put;
> +
> + elem_size = map->key_size + value_size;
> + if (buf_len < elem_size) {
> + err = -EINVAL;
> + goto err_put;
> + }
> +
> + if (ukey) {
> + prev_key = __bpf_copy_key(ukey, map->key_size);
> + if (IS_ERR(prev_key)) {
> + err = PTR_ERR(prev_key);
> + goto err_put;
> + }
> + } else {
> + prev_key = NULL;
> + first_key = true;
> + }
> +
> + err = -ENOMEM;
> + buf = kmalloc(elem_size, GFP_USER | __GFP_NOWARN);
> + if (!buf)
> + goto err_put;
> +
> + key = buf;
> + value = key + map->key_size;
> + for (cp_len = 0; cp_len + elem_size <= buf_len;) {
> + if (signal_pending(current)) {
> + err = -EINTR;
> + break;
> + }
> +
> + rcu_read_lock();
> + err = map->ops->map_get_next_key(map, prev_key, key);
> + rcu_read_unlock();
> +
> + if (err)
> + break;
> +
> + err = bpf_map_copy_value(map, key, value, attr->dump.flags);
> +
> + if (err == -ENOENT)
> + continue;
> + if (err)
> + goto free_buf;
> +
> + if (copy_to_user(ubuf + cp_len, buf, elem_size)) {
> + err = -EFAULT;
> + goto free_buf;
> + }
> +
> + prev_key = key;
> + cp_len += elem_size;
> + }
> +
> + if (err == -ENOENT && cp_len)
> + err = 0;
> + if (!err && (copy_to_user(ubuf_len, &cp_len, sizeof(cp_len)) ||
> + (!first_key && copy_to_user(ukey, key, map->key_size))))
> + err = -EFAULT;
> +free_buf:
> + kfree(buf);
> +err_put:
> + fdput(f);
> + return err;
> +}
> +
> #define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD value
>
> static int map_lookup_and_delete_elem(union bpf_attr *attr)
> @@ -2910,6 +3025,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
> case BPF_MAP_LOOKUP_AND_DELETE_ELEM:
> err = map_lookup_and_delete_elem(&attr);
> break;
> + case BPF_MAP_DUMP:
> + err = map_dump(&attr);
> + break;
In bcc, we have use cases like this. At a certain time interval (e.g.,
every 2 seconds),
we get all key/value pairs for a map, we format and print out map
key/values on the screen,
and then delete all key/value pairs we retrieved earlier.
Currently, bpf_get_next_key() is used to get all key/value pairs, and
deletion also happened
at each key level.
Your batch dump command should help retrieving map key/value pairs.
What do you think deletions of those just retrieved map entries?
With an additional flag and fold into BPF_MAP_DUMP?
or implement a new BPF_MAP_DUMP_AND_DELETE?
I mentioned this so that we can start discussion now.
You do not need to implement batch deletion part, but let us
have a design extensible for that.
Thanks.
> default:
> err = -EINVAL;
> break;
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH net-next] nfp: tls: fix error return code in nfp_net_tls_add()
From: Jakub Kicinski @ 2019-07-05 16:21 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Dirk van der Merwe, oss-drivers, netdev, kernel-janitors
In-Reply-To: <20190705082625.168515-1-weiyongjun1@huawei.com>
On Fri, 5 Jul 2019 08:26:25 +0000, Wei Yongjun wrote:
> Fix to return negative error code -EINVAL from the error handling
> case instead of 0, as done elsewhere in this function.
>
> Fixes: 1f35a56cf586 ("nfp: tls: add/delete TLS TX connections")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: [PATCH v7 net-next 1/5] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Saeed Mahameed @ 2019-07-05 16:19 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Ivan Khoronzhuk, grygorii.strashko, David S. Miller,
Alexei Starovoitov, linux-kernel, linux-omap, ilias.apalodimas,
Linux Netdev List, Daniel Borkmann, Jakub Kicinski,
John Fastabend, Tariq Toukan, Saeed Mahameed
In-Reply-To: <20190705094346.13b06da6@carbon>
On Fri, Jul 5, 2019 at 3:45 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
>
> CC: Tariq + Saeed, could you please review the mlx5 part.
>
> On Fri, 5 Jul 2019 02:14:02 +0300
> Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
> > Jesper recently removed page_pool_destroy() (from driver invocation)
> > and moved shutdown and free of page_pool into xdp_rxq_info_unreg(),
> > in-order to handle in-flight packets/pages. This created an asymmetry
> > in drivers create/destroy pairs.
> >
> > This patch reintroduce page_pool_destroy and add page_pool user
> > refcnt. This serves the purpose to simplify drivers error handling as
> > driver now drivers always calls page_pool_destroy() and don't need to
> > track if xdp_rxq_info_reg_mem_model() was unsuccessful.
> >
> > This could be used for a special cases where a single RX-queue (with a
> > single page_pool) provides packets for two net_device'es, and thus
> > needs to register the same page_pool twice with two xdp_rxq_info
> > structures.
> >
> > This patch is primarily to ease API usage for drivers. The recently
> > merged netsec driver, actually have a bug in this area, which is
> > solved by this API change.
> >
This patch and the mlx5 part looks good to me,
Reviewed-by: Saeed Mahameed <saeedm@mellanox.com>
> > This patch is a modified version of Ivan Khoronzhu's original patch.
> >
> > Link: https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
> > Fixes: 5c67bf0ec4d0 ("net: netsec: Use page_pool API")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> Thank you Ivan for taking this more simple approach. If we later see
> more drivers wanting this feature of a single RX-queue providing
> packets to multiple net_device'es, then we can change into your
> more generic API at XDP-reg-layer approach later. For now, we keep
> code complexity as low as possible.
>
>
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_main.c | 4 +--
> > drivers/net/ethernet/socionext/netsec.c | 8 ++----
> > include/net/page_pool.h | 25 +++++++++++++++++++
> > net/core/page_pool.c | 8 ++++++
> > net/core/xdp.c | 3 +++
> > 5 files changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index 2f9093ba82aa..ac882b2341d0 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -575,8 +575,6 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
> > }
> > err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq,
> > MEM_TYPE_PAGE_POOL, rq->page_pool);
> > - if (err)
> > - page_pool_free(rq->page_pool);
> > }
> > if (err)
> > goto err_free;
> > @@ -644,6 +642,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
> > if (rq->xdp_prog)
> > bpf_prog_put(rq->xdp_prog);
> > xdp_rxq_info_unreg(&rq->xdp_rxq);
> > + page_pool_destroy(rq->page_pool);
> > mlx5_wq_destroy(&rq->wq_ctrl);
> >
> > return err;
> > @@ -678,6 +677,7 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> > }
> >
> > xdp_rxq_info_unreg(&rq->xdp_rxq);
> > + page_pool_destroy(rq->page_pool);
> > mlx5_wq_destroy(&rq->wq_ctrl);
> > }
> >
> > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> > index 5544a722543f..43ab0ce90704 100644
> > --- a/drivers/net/ethernet/socionext/netsec.c
> > +++ b/drivers/net/ethernet/socionext/netsec.c
> > @@ -1210,15 +1210,11 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id)
> > }
> > }
> >
> > - /* Rx is currently using page_pool
> > - * since the pool is created during netsec_setup_rx_dring(), we need to
> > - * free the pool manually if the registration failed
> > - */
> > + /* Rx is currently using page_pool */
> > if (id == NETSEC_RING_RX) {
> > if (xdp_rxq_info_is_reg(&dring->xdp_rxq))
> > xdp_rxq_info_unreg(&dring->xdp_rxq);
> > - else
> > - page_pool_free(dring->page_pool);
> > + page_pool_destroy(dring->page_pool);
> > }
> >
> > memset(dring->desc, 0, sizeof(struct netsec_desc) * DESC_NUM);
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index ee9c871d2043..2cbcdbdec254 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -101,6 +101,12 @@ struct page_pool {
> > struct ptr_ring ring;
> >
> > atomic_t pages_state_release_cnt;
> > +
> > + /* A page_pool is strictly tied to a single RX-queue being
> > + * protected by NAPI, due to above pp_alloc_cache. This
> > + * refcnt serves purpose is to simplify drivers error handling.
> > + */
> > + refcount_t user_cnt;
> > };
> >
> > struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
> > @@ -134,6 +140,15 @@ static inline void page_pool_free(struct page_pool *pool)
> > #endif
> > }
> >
> > +/* Drivers use this instead of page_pool_free */
> > +static inline void page_pool_destroy(struct page_pool *pool)
> > +{
> > + if (!pool)
> > + return;
> > +
> > + page_pool_free(pool);
> > +}
> > +
> > /* Never call this directly, use helpers below */
> > void __page_pool_put_page(struct page_pool *pool,
> > struct page *page, bool allow_direct);
> > @@ -201,4 +216,14 @@ static inline bool is_page_pool_compiled_in(void)
> > #endif
> > }
> >
> > +static inline void page_pool_get(struct page_pool *pool)
> > +{
> > + refcount_inc(&pool->user_cnt);
> > +}
> > +
> > +static inline bool page_pool_put(struct page_pool *pool)
> > +{
> > + return refcount_dec_and_test(&pool->user_cnt);
> > +}
> > +
> > #endif /* _NET_PAGE_POOL_H */
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index b366f59885c1..3272dc7a8c81 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -49,6 +49,9 @@ static int page_pool_init(struct page_pool *pool,
> >
> > atomic_set(&pool->pages_state_release_cnt, 0);
> >
> > + /* Driver calling page_pool_create() also call page_pool_destroy() */
> > + refcount_set(&pool->user_cnt, 1);
> > +
> > if (pool->p.flags & PP_FLAG_DMA_MAP)
> > get_device(pool->p.dev);
> >
> > @@ -70,6 +73,7 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
> > kfree(pool);
> > return ERR_PTR(err);
> > }
> > +
> > return pool;
> > }
> > EXPORT_SYMBOL(page_pool_create);
> > @@ -356,6 +360,10 @@ static void __warn_in_flight(struct page_pool *pool)
> >
> > void __page_pool_free(struct page_pool *pool)
> > {
> > + /* Only last user actually free/release resources */
> > + if (!page_pool_put(pool))
> > + return;
> > +
> > WARN(pool->alloc.count, "API usage violation");
> > WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 829377cc83db..d7bf62ffbb5e 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -370,6 +370,9 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> > goto err;
> > }
> >
> > + if (type == MEM_TYPE_PAGE_POOL)
> > + page_pool_get(xdp_alloc->page_pool);
> > +
> > mutex_unlock(&mem_id_lock);
> >
> > trace_mem_connect(xdp_alloc, xdp_rxq);
>
>
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ 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