On Wed, May 22, 2019 at 6:38 AM Björn Töpel <bjorn.topel@xxxxxxxxx> wrote:
>
> From: Björn Töpel <bjorn.topel@xxxxxxxxx>
>
> When an AF_XDP socket is released/closed the XSKMAP still holds a
> reference to the socket in a "released" state. The socket will still
> use the netdev queue resource, and block newly created sockets from
> attaching to that queue, but no user application can access the
> fill/complete/rx/tx queues. This results in that all applications need
> to explicitly clear the map entry from the old "zombie state"
> socket. This should be done automatically.
>
> After this patch, when a socket is released, it will remove itself
> from all the XSKMAPs it resides in, allowing the socket application to
> remove the code that cleans the XSKMAP entry.
>
> This behavior is also closer to that of SOCKMAP, making the two socket
> maps more consistent.
>
> Suggested-by: Bruce Richardson <bruce.richardson@xxxxxxxxx>
> Signed-off-by: Björn Töpel <bjorn.topel@xxxxxxxxx>
> ---
> include/net/xdp_sock.h | 3 ++
> kernel/bpf/xskmap.c | 101 +++++++++++++++++++++++++++++++++++------
> net/xdp/xsk.c | 25 ++++++++++
> 3 files changed, 116 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index d074b6d60f8a..b5f8f9f826d0 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -68,6 +68,8 @@ struct xdp_sock {
> */
> spinlock_t tx_completion_lock;
> u64 rx_dropped;
> + struct list_head map_list;
> + spinlock_t map_list_lock;
> };
>
> struct xdp_buff;
> @@ -87,6 +89,7 @@ struct xdp_umem_fq_reuse *xsk_reuseq_swap(struct xdp_umem *umem,
> struct xdp_umem_fq_reuse *newq);
> void xsk_reuseq_free(struct xdp_umem_fq_reuse *rq);
> struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev, u16 queue_id);
> +void xsk_map_delete_from_node(struct xdp_sock *xs, struct list_head *node);
>
> static inline char *xdp_umem_get_data(struct xdp_umem *umem, u64 addr)
> {
> diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c
> index 686d244e798d..318f6a07fa31 100644
> --- a/kernel/bpf/xskmap.c
> +++ b/kernel/bpf/xskmap.c
> @@ -13,8 +13,58 @@ struct xsk_map {
> struct bpf_map map;
> struct xdp_sock **xsk_map;
> struct list_head __percpu *flush_list;
> + spinlock_t lock;
> };
>
> +/* Nodes are linked in the struct xdp_sock map_list field, and used to
> + * track which maps a certain socket reside in.
> + */
> +struct xsk_map_node {
> + struct list_head node;
> + struct xsk_map *map;
> + struct xdp_sock **map_entry;
> +};
Why do we need map_entry to be struct xdp_sock **? I think we could
just use struct xdp_sock *? Or did I miss anytihg?
Thanks,
Song
> +
> +static struct xsk_map_node *xsk_map_node_alloc(void)
> +{
> + return kzalloc(sizeof(struct xsk_map_node), GFP_ATOMIC | __GFP_NOWARN);
> +}
> +
> +static void xsk_map_node_free(struct xsk_map_node *node)
> +{
> + kfree(node);
> +}
> +
> +static void xsk_map_node_init(struct xsk_map_node *node,
> + struct xsk_map *map,
> + struct xdp_sock **map_entry)
> +{
> + node->map = map;
> + node->map_entry = map_entry;
> +}
> +
> +static void xsk_map_add_node(struct xdp_sock *xs, struct xsk_map_node *node)
> +{
> + spin_lock_bh(&xs->map_list_lock);
> + list_add_tail(&node->node, &xs->map_list);
> + spin_unlock_bh(&xs->map_list_lock);
> +}
> +
> +static void xsk_map_del_node(struct xdp_sock *xs, struct xdp_sock **map_entry)
> +{
> + struct xsk_map_node *n, *tmp;
> +
> + spin_lock_bh(&xs->map_list_lock);
> + list_for_each_entry_safe(n, tmp, &xs->map_list, node) {
> + if (map_entry == n->map_entry) {
> + list_del(&n->node);
> + xsk_map_node_free(n);
> + }
> + }
> + spin_unlock_bh(&xs->map_list_lock);
> +
> +}
> +
> static struct bpf_map *xsk_map_alloc(union bpf_attr *attr)
> {
> int cpu, err = -EINVAL;
> @@ -34,6 +84,7 @@ static struct bpf_map *xsk_map_alloc(union bpf_attr *attr)
> return ERR_PTR(-ENOMEM);
>
> bpf_map_init_from_attr(&m->map, attr);
> + spin_lock_init(&m->lock);
>
> cost = (u64)m->map.max_entries * sizeof(struct xdp_sock *);
> cost += sizeof(struct list_head) * num_possible_cpus();
> @@ -78,15 +129,16 @@ static void xsk_map_free(struct bpf_map *map)
> bpf_clear_redirect_map(map);
> synchronize_net();
>
> + spin_lock_bh(&m->lock);
> for (i = 0; i < map->max_entries; i++) {
> - struct xdp_sock *xs;
> -
> - xs = m->xsk_map[i];
> - if (!xs)
> - continue;
> + struct xdp_sock **map_entry = &m->xsk_map[i];
> + struct xdp_sock *old_xs;
>
> - sock_put((struct sock *)xs);
> + old_xs = xchg(map_entry, NULL);
> + if (old_xs)
> + xsk_map_del_node(old_xs, map_entry);
> }
> + spin_unlock_bh(&m->lock);
>
> free_percpu(m->flush_list);
> bpf_map_area_free(m->xsk_map);
> @@ -162,7 +214,8 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> {
> struct xsk_map *m = container_of(map, struct xsk_map, map);
> u32 i = *(u32 *)key, fd = *(u32 *)value;
> - struct xdp_sock *xs, *old_xs;
> + struct xdp_sock *xs, *old_xs, **entry;
> + struct xsk_map_node *node;
> struct socket *sock;
> int err;
>
> @@ -189,11 +242,20 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> return -EOPNOTSUPP;
> }
>
> - sock_hold(sock->sk);
> + node = xsk_map_node_alloc();
> + if (!node) {
> + sockfd_put(sock);
> + return -ENOMEM;
> + }
>
> - old_xs = xchg(&m->xsk_map[i], xs);
> + spin_lock_bh(&m->lock);
> + entry = &m->xsk_map[i];
> + xsk_map_node_init(node, m, entry);
> + xsk_map_add_node(xs, node);
> + old_xs = xchg(entry, xs);
> if (old_xs)
> - sock_put((struct sock *)old_xs);
> + xsk_map_del_node(old_xs, entry);
> + spin_unlock_bh(&m->lock);
>
> sockfd_put(sock);
> return 0;
> @@ -202,19 +264,32 @@ static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value,
> static int xsk_map_delete_elem(struct bpf_map *map, void *key)
> {
> struct xsk_map *m = container_of(map, struct xsk_map, map);
> - struct xdp_sock *old_xs;
> + struct xdp_sock *old_xs, **map_entry;
> int k = *(u32 *)key;
>
> if (k >= map->max_entries)
> return -EINVAL;
>
> - old_xs = xchg(&m->xsk_map[k], NULL);
> + spin_lock_bh(&m->lock);
> + map_entry = &m->xsk_map[k];
> + old_xs = xchg(map_entry, NULL);
> if (old_xs)
> - sock_put((struct sock *)old_xs);
> + xsk_map_del_node(old_xs, map_entry);
> + spin_unlock_bh(&m->lock);
>
> return 0;
> }
>
> +void xsk_map_delete_from_node(struct xdp_sock *xs, struct list_head *node)
> +{
> + struct xsk_map_node *n = list_entry(node, struct xsk_map_node, node);
> +
> + spin_lock_bh(&n->map->lock);
> + *n->map_entry = NULL;
> + spin_unlock_bh(&n->map->lock);
> + xsk_map_node_free(n);
> +}
> +
> const struct bpf_map_ops xsk_map_ops = {
> .map_alloc = xsk_map_alloc,
> .map_free = xsk_map_free,
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..1931d98a7754 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -335,6 +335,27 @@ static int xsk_init_queue(u32 entries, struct xsk_queue **queue,
> return 0;
> }
>
> +static struct list_head *xsk_map_list_pop(struct xdp_sock *xs)
> +{
> + struct list_head *node = NULL;
> +
> + spin_lock_bh(&xs->map_list_lock);
> + if (!list_empty(&xs->map_list)) {
> + node = xs->map_list.next;
> + list_del(node);
> + }
> + spin_unlock_bh(&xs->map_list_lock);
> + return node;
> +}
> +
> +static void xsk_delete_from_maps(struct xdp_sock *xs)
> +{
> + struct list_head *node;
> +
> + while ((node = xsk_map_list_pop(xs)))
> + xsk_map_delete_from_node(xs, node);
> +}
> +
> static int xsk_release(struct socket *sock)
> {
> struct sock *sk = sock->sk;
> @@ -354,6 +375,7 @@ static int xsk_release(struct socket *sock)
> sock_prot_inuse_add(net, sk->sk_prot, -1);
> local_bh_enable();
>
> + xsk_delete_from_maps(xs);
> if (xs->dev) {
> struct net_device *dev = xs->dev;
>
> @@ -767,6 +789,9 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
> mutex_init(&xs->mutex);
> spin_lock_init(&xs->tx_completion_lock);
>
> + INIT_LIST_HEAD(&xs->map_list);
> + spin_lock_init(&xs->map_list_lock);
> +
> mutex_lock(&net->xdp.lock);
> sk_add_node_rcu(sk, &net->xdp.list);
> mutex_unlock(&net->xdp.lock);
> --
> 2.20.1
>
