On Tue, 4 Jun 2019 10:07:25 +0200
Björn Töpel [off-list ref] wrote:

On 2019-06-03 15:19, Maciej Fijalkowski wrote:

quoted

Since xsk support in libbpf loads the xdp program interface, make it
also responsible for its removal. Store the prog id in xsk_socket_config
so when removing the program we are still able to compare the current
program id with the id from the attachment time and make a decision
onward.

While at it, remove the socket/umem in xdpsock's error path.

We're loading a new, or reusing an existing XDP program at socket
creation, but tearing it down at *socket delete* is explicitly left to
the application.

Are you describing here the old behavior?

For a per-queue XDP program (tied to the socket), this kind cleanup would
make sense.

The intention with the libbpf AF_XDP support was to leave the XDP
handling to whatever XDP orchestration process availble. It's not part
of libbpf. For convenience, *loading/lookup of the XDP program* was
added even though this was an asymmetry.

Hmmm ok and I tried to make it symmetric :p 

For the sample application, this makes sense, but for larger/real
applications?

Tough questions on those real apps!

OTOH I like the idea of a scoped cleanup "when all sockets are gone",
the XDP program + maps are removed.

That's happening with patch 4 included from this set (in case it gets fixed :))

quoted

Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
  samples/bpf/xdpsock_user.c | 33 ++++++++++-----------------------
  tools/lib/bpf/xsk.c        | 32 ++++++++++++++++++++++++++++++++
  tools/lib/bpf/xsk.h        |  1 +
  3 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
index e9dceb09b6d1..123862b16dd4 100644
--- a/samples/bpf/xdpsock_user.c
+++ b/samples/bpf/xdpsock_user.c

@@ -68,7 +68,6 @@ static int opt_queue;
  static int opt_poll;
  static int opt_interval = 1;
  static u32 opt_xdp_bind_flags;
-static __u32 prog_id;
  
  struct xsk_umem_info {
  	struct xsk_ring_prod fq;

@@ -170,22 +169,6 @@ static void *poller(void *arg)
  	return NULL;
  }
  
-static void remove_xdp_program(void)
-{
-	__u32 curr_prog_id = 0;
-
-	if (bpf_get_link_xdp_id(opt_ifindex, &curr_prog_id, opt_xdp_flags)) {
-		printf("bpf_get_link_xdp_id failed\n");
-		exit(EXIT_FAILURE);
-	}
-	if (prog_id == curr_prog_id)
-		bpf_set_link_xdp_fd(opt_ifindex, -1, opt_xdp_flags);
-	else if (!curr_prog_id)
-		printf("couldn't find a prog id on a given interface\n");
-	else
-		printf("program on interface changed, not removing\n");
-}
-
  static void int_exit(int sig)
  {
  	struct xsk_umem *umem = xsks[0]->umem->umem;

@@ -195,7 +178,6 @@ static void int_exit(int sig)
  	dump_stats();
  	xsk_socket__delete(xsks[0]->xsk);
  	(void)xsk_umem__delete(umem);
-	remove_xdp_program();
  
  	exit(EXIT_SUCCESS);
  }

@@ -206,7 +188,16 @@ static void __exit_with_error(int error, const char *file, const char *func,
  	fprintf(stderr, "%s:%s:%i: errno: %d/\"%s\"\n", file, func,
  		line, error, strerror(error));
  	dump_stats();
-	remove_xdp_program();
+
+	if (xsks[0]->xsk)
+		xsk_socket__delete(xsks[0]->xsk);
+
+	if (xsks[0]->umem) {
+		struct xsk_umem *umem = xsks[0]->umem->umem;
+
+		(void)xsk_umem__delete(umem);
+	}
+
  	exit(EXIT_FAILURE);
  }
  

@@ -312,10 +303,6 @@ static struct xsk_socket_info *xsk_configure_socket(struct xsk_umem_info *umem)
  	if (ret)
  		exit_with_error(-ret);
  
-	ret = bpf_get_link_xdp_id(opt_ifindex, &prog_id, opt_xdp_flags);
-	if (ret)
-		exit_with_error(-ret);
-
  	return xsk;
  }
  

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 514ab3fb06f4..e28bedb0b078 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c

@@ -259,6 +259,8 @@ int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,
  static int xsk_load_xdp_prog(struct xsk_socket *xsk)
  {
  	static const int log_buf_size = 16 * 1024;
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
  	char log_buf[log_buf_size];
  	int err, prog_fd;
  

@@ -321,6 +323,14 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk)
  		return err;
  	}
  
+	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+	if (err) {
+		pr_warning("can't get prog info - %s\n", strerror(errno));
+		close(prog_fd);
+		return err;
+	}
+	xsk->config.prog_id = info.id;
+
  	xsk->prog_fd = prog_fd;
  	return 0;
  }

@@ -483,6 +493,25 @@ static int xsk_set_bpf_maps(struct xsk_socket *xsk)
  	return err;
  }
  
+static void xsk_remove_xdp_prog(struct xsk_socket *xsk)
+{
+	__u32 prog_id = xsk->config.prog_id;
+	__u32 curr_prog_id = 0;
+	int err;
+
+	err = bpf_get_link_xdp_id(xsk->ifindex, &curr_prog_id,
+				  xsk->config.xdp_flags);
+	if (err)
+		return;
+
+	if (prog_id == curr_prog_id)
+		bpf_set_link_xdp_fd(xsk->ifindex, -1, xsk->config.xdp_flags);
+	else if (!curr_prog_id)
+		pr_warning("couldn't find a prog id on a given interface\n");
+	else
+		pr_warning("program on interface changed, not removing\n");
+}
+
  static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
  {
  	__u32 prog_id = 0;

@@ -506,6 +535,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk)
  		err = xsk_lookup_bpf_maps(xsk);
  		if (err)
  			goto out_load;
+		xsk->config.prog_id = prog_id;
  	}
  
  	err = xsk_set_bpf_maps(xsk);

@@ -744,6 +774,8 @@ void xsk_socket__delete(struct xsk_socket *xsk)
  
  	}
  
+	xsk_remove_xdp_prog(xsk);
+
  	xsk->umem->refcount--;
  	/* Do not close an fd that also has an associated umem connected
  	 * to it.

diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 82ea71a0f3ec..e1b23e9432c9 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h

@@ -186,6 +186,7 @@ struct xsk_socket_config {
  	__u32 tx_size;
  	__u32 libbpf_flags;
  	__u32 xdp_flags;
+	__u32 prog_id;
  	__u16 bind_flags;
  };