Hi Shibin

I will add a more detailed note re CAP_BPF in the documentation on the V2.

BR
Maryam


On 30/11/2023 13:56, Koikkara Reeny, Shibin wrote:
Hi Maryam,

I have one more question.

Regards,
Shibin

-----Original Message-----
From: Koikkara Reeny, Shibin <shibin.koikkara.re...@intel.com>
Sent: Thursday, November 30, 2023 12:14 PM
To: Tahhan, Maryam <mtah...@redhat.com>; ferruh.yi...@amd.com;
step...@networkplumber.org; lihuis...@huawei.com;
fengcheng...@huawei.com; liuyongl...@huawei.com
Cc: dev@dpdk.org; Tahhan, Maryam <mtah...@redhat.com>
Subject: RE: [v1] net/af_xdp: enable a sock path alongside use_cni

Hi Maryam,

I have added some suggestion below.

Regrads,
Shibin

-----Original Message-----
From: Maryam Tahhan <mtah...@redhat.com>
Sent: Thursday, November 30, 2023 9:14 AM
To: ferruh.yi...@amd.com; step...@networkplumber.org;
lihuis...@huawei.com; fengcheng...@huawei.com;
liuyongl...@huawei.com
Cc: dev@dpdk.org; Tahhan, Maryam <mtah...@redhat.com>
Subject: [v1] net/af_xdp: enable a sock path alongside use_cni

With the original 'use_cni' implementation, (using a hardcoded socket
rather than a configurable one), if a single pod is requesting
multiple net devices and these devices are from different pools, then
the container attempts to mount all the netdev UDSes in the pod as
/tmp/afxdp.sock. Which means that at best only 1 netdev will handshake
correctly with the AF_XDP DP. This patch addresses this by making the
socket parameter configurable alongside the 'use_cni' param.
Tested with the AF_XDP DP CNI PR 81.

Signed-off-by: Maryam Tahhan <mtah...@redhat.com>
---
  doc/guides/howto/af_xdp_cni.rst     | 18 +++++++---
  drivers/net/af_xdp/rte_eth_af_xdp.c | 56
+++++++++++++++++++----------
  2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/doc/guides/howto/af_xdp_cni.rst
b/doc/guides/howto/af_xdp_cni.rst index a1a6d5b99c..a2d90c665d 100644
--- a/doc/guides/howto/af_xdp_cni.rst
+++ b/doc/guides/howto/af_xdp_cni.rst
@@ -38,9 +38,10 @@ The XSKMAP is a BPF map of AF_XDP sockets (XSK).
  The client can then proceed with creating an AF_XDP socket  and
inserting that socket into the XSKMAP pointed to by the descriptor.

-The EAL vdev argument ``use_cni`` is used to indicate that the user
wishes
+The EAL vdev arguments ``use_cni`` and ``sock`` are used to indicate
+that the user wishes
  to run the PMD in unprivileged mode and to receive the XSKMAP file
descriptor  from the CNI.
+
  When this flag is set,
  the ``XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD`` libbpf flag  should be
used when creating the socket @@ -49,7 +50,7 @@ Instead the loading is
handled by the CNI.

  .. note::

-   The Unix Domain Socket file path appear in the end user is
"/tmp/afxdp.sock".
+   The Unix Domain Socket file path appears to the end user at
"/tmp/afxdp_dp/<netdev>/afxdp.sock".


  Prerequisites
@@ -224,7 +225,6 @@ Howto run dpdk-testpmd with CNI plugin:
            capabilities:
               add:
                 - CAP_NET_RAW
-               - CAP_BPF
Why the CAP_BPF is removed?

           resources:
             requests:
               hugepages-2Mi: 2Gi
@@ -245,7 +245,17 @@ Howto run dpdk-testpmd with CNI plugin:

       kubectl exec -i <Pod name> --container <containers name> -- \
             /<Path>/dpdk-testpmd -l 0,1 --no-pci \
-           --vdev=net_af_xdp0,use_cni=1,iface=<interface name> \
+           --vdev=net_af_xdp0,use_cni=1,iface=<interface
name>,sock=/tmp/afxdp_dp/<interface name>/afxdp.sock \
+           -- --no-mlockall --in-memory
+
+for multiple devices use:
+
+  .. code-block:: console
+
+     kubectl exec -i <Pod name> --container <containers name> -- \
+           /<Path>/dpdk-testpmd -l 0-2 --no-pci \
+           --vdev=net_af_xdp0,use_cni=1,iface=<interface
name>,sock=/tmp/afxdp_dp/<interface name>/afxdp.sock \
+           --vdev=net_af_xdp1,use_cni=1,iface=<interface
+ name>,sock=/tmp/afxdp_dp/<interface name>/afxdp.sock \
             -- --no-mlockall --in-memory

  For further reference please use the `e2e`_ test case in `AF_XDP
Plugin for Kubernetes`_ diff --git
a/drivers/net/af_xdp/rte_eth_af_xdp.c
b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 353c8688ec..f728dae2f9 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -88,7 +88,6 @@ RTE_LOG_REGISTER_DEFAULT(af_xdp_logtype,
NOTICE);
  #define UDS_MAX_CMD_LEN                       64
  #define UDS_MAX_CMD_RESP              128
  #define UDS_XSK_MAP_FD_MSG            "/xsk_map_fd"
-#define UDS_SOCK                       "/tmp/afxdp.sock"
  #define UDS_CONNECT_MSG                       "/connect"
  #define UDS_HOST_OK_MSG                       "/host_ok"
  #define UDS_HOST_NAK_MSG              "/host_nak"
@@ -171,6 +170,7 @@ struct pmd_internals {
        bool custom_prog_configured;
        bool force_copy;
        bool use_cni;
+       char sock_path[PATH_MAX];
I would recommend using variable name as "uds_path".

        struct bpf_map *map;

        struct rte_ether_addr eth_addr;
@@ -191,6 +191,7 @@ struct pmd_process_private {
  #define ETH_AF_XDP_BUDGET_ARG
        "busy_budget"
  #define ETH_AF_XDP_FORCE_COPY_ARG             "force_copy"
  #define ETH_AF_XDP_USE_CNI_ARG                        "use_cni"
+#define ETH_AF_XDP_SOCK_ARG                    "sock"
To make it clear would  recommend using "sock_path" and also
ETH_AF_XDP_CNI_UDS_PATH_ARG or ETH_AF_XDP_SOCK_PATH_ARG.

  static const char * const valid_arguments[] = {
        ETH_AF_XDP_IFACE_ARG,
@@ -201,6 +202,7 @@ static const char * const valid_arguments[] = {
        ETH_AF_XDP_BUDGET_ARG,
        ETH_AF_XDP_FORCE_COPY_ARG,
        ETH_AF_XDP_USE_CNI_ARG,
+       ETH_AF_XDP_SOCK_ARG,
        NULL
  };

@@ -1351,7 +1353,7 @@ configure_preferred_busy_poll(struct
pkt_rx_queue *rxq)  }

  static int
-init_uds_sock(struct sockaddr_un *server)
+init_uds_sock(struct sockaddr_un *server, const char *sock_path)
  {
        int sock;

@@ -1362,7 +1364,7 @@ init_uds_sock(struct sockaddr_un *server)
        }

        server->sun_family = AF_UNIX;
-       strlcpy(server->sun_path, UDS_SOCK, sizeof(server->sun_path));
+       strlcpy(server->sun_path, sock_path, sizeof(server->sun_path));

        if (connect(sock, (struct sockaddr *)server, sizeof(struct
sockaddr_un)) < 0) {
                close(sock);
@@ -1382,7 +1384,7 @@ struct msg_internal {  };

  static int
-send_msg(int sock, char *request, int *fd)
+send_msg(int sock, char *request, int *fd, const char *sock_path)
  {
        int snd;
        struct iovec iov;
@@ -1393,7 +1395,7 @@ send_msg(int sock, char *request, int *fd)

        memset(&dst, 0, sizeof(dst));
        dst.sun_family = AF_UNIX;
-       strlcpy(dst.sun_path, UDS_SOCK, sizeof(dst.sun_path));
+       strlcpy(dst.sun_path, sock_path, sizeof(dst.sun_path));

        /* Initialize message header structure */
        memset(&msgh, 0, sizeof(msgh));
@@ -1471,7 +1473,7 @@ read_msg(int sock, char *response, struct
sockaddr_un *s, int *fd)

  static int
  make_request_cni(int sock, struct sockaddr_un *server, char *request,
-                int *req_fd, char *response, int *out_fd)
+                int *req_fd, char *response, int *out_fd, const char
*sock_path)
  {
        int rval;

@@ -1483,7 +1485,7 @@ make_request_cni(int sock, struct sockaddr_un
*server, char *request,
        if (req_fd == NULL)
                rval = write(sock, request, strlen(request));
        else
-               rval = send_msg(sock, request, req_fd);
+               rval = send_msg(sock, request, req_fd, sock_path);

        if (rval < 0) {
                AF_XDP_LOG(ERR, "Write error %s\n", strerror(errno)); @@
-1507,7 +1509,7 @@ check_response(char *response, char *exp_resp,
long
size)  }

  static int
-get_cni_fd(char *if_name)
+get_cni_fd(char *if_name, const char *sock_path)
  {
        char request[UDS_MAX_CMD_LEN],
response[UDS_MAX_CMD_RESP];
        char hostname[MAX_LONG_OPT_SZ],
exp_resp[UDS_MAX_CMD_RESP]; @@ -1520,14 +1522,14 @@
get_cni_fd(char
*if_name)
                return -1;

        memset(&server, 0, sizeof(server));
-       sock = init_uds_sock(&server);
+       sock = init_uds_sock(&server, sock_path);
        if (sock < 0)
                return -1;

        /* Initiates handshake to CNI send: /connect,hostname */
        snprintf(request, sizeof(request), "%s,%s", UDS_CONNECT_MSG,
hostname);
        memset(response, 0, sizeof(response));
-       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd) < 0) {
+       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd,
+sock_path) < 0) {
Why do we need to pass "sock_path" here as we have already connected
the sock with sock_path in init_uds_sock()?

                AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n",
request);
                goto err_close;
        }
@@ -1541,7 +1543,7 @@ get_cni_fd(char *if_name)
        /* Request for "/version" */
        strlcpy(request, UDS_VERSION_MSG, UDS_MAX_CMD_LEN);
        memset(response, 0, sizeof(response));
-       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd) < 0) {
+       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd,
+sock_path) < 0) {
Same question as above.

                AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n",
request);
                goto err_close;
        }
@@ -1549,7 +1551,7 @@ get_cni_fd(char *if_name)
        /* Request for file descriptor for netdev name*/
        snprintf(request, sizeof(request), "%s,%s",
UDS_XSK_MAP_FD_MSG,
if_name);
        memset(response, 0, sizeof(response));
-       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd) < 0) {
+       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd,
+sock_path) < 0) {
Same question as above.

                AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n",
request);
                goto err_close;
        }
@@ -1571,7 +1573,7 @@ get_cni_fd(char *if_name)
        /* Initiate close connection */
        strlcpy(request, UDS_FIN_MSG, UDS_MAX_CMD_LEN);
        memset(response, 0, sizeof(response));
-       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd) < 0) {
+       if (make_request_cni(sock, &server, request, NULL, response,
&out_fd,
+sock_path) < 0) {
Same question as above.

                AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n",
request);
                goto err_close;
        }
@@ -1698,7 +1700,7 @@ xsk_configure(struct pmd_internals *internals,
struct pkt_rx_queue *rxq,
                int err, fd, map_fd;

                /* get socket fd from CNI plugin */
-               map_fd = get_cni_fd(internals->if_name);
+               map_fd = get_cni_fd(internals->if_name, internals-
sock_path);
                if (map_fd < 0) {
                        AF_XDP_LOG(ERR, "Failed to receive CNI plugin
fd\n");
                        goto out_xsk;
@@ -2023,7 +2025,8 @@ xdp_get_channels_info(const char *if_name, int
*max_queues,  static int  parse_parameters(struct rte_kvargs *kvlist,
char *if_name, int *start_queue,
                 int *queue_cnt, int *shared_umem, char *prog_path,
-                int *busy_budget, int *force_copy, int *use_cni)
+                int *busy_budget, int *force_copy, int *use_cni,
+                char *sock_path)
  {
        int ret;

@@ -2069,6 +2072,11 @@ parse_parameters(struct rte_kvargs *kvlist,
char *if_name, int *start_queue,
        if (ret < 0)
                goto free_kvlist;

+       ret = rte_kvargs_process(kvlist, ETH_AF_XDP_SOCK_ARG,
+                                &parse_prog_arg, sock_path);
Parse_prog_arg does 2 things copy the sock_arg value and also check the
access to the socket.
Checking access here has a chance of causing raise condition so I would
recommend to skip this check here as this will be taken care in the
init_uds_sock().

+       if (ret < 0)
+               goto free_kvlist;
+
  free_kvlist:
        rte_kvargs_free(kvlist);
        return ret;
@@ -2108,7 +2116,7 @@ static struct rte_eth_dev *
init_internals(struct rte_vdev_device *dev, const char *if_name,
               int start_queue_idx, int queue_cnt, int shared_umem,
               const char *prog_path, int busy_budget, int force_copy,
-              int use_cni)
+              int use_cni, const char *sock_path)
  {
        const char *name = rte_vdev_device_name(dev);
        const unsigned int numa_node = dev->device.numa_node; @@ -
2138,6 +2146,7 @@ init_internals(struct rte_vdev_device *dev, const
char *if_name,
        internals->shared_umem = shared_umem;
        internals->force_copy = force_copy;
        internals->use_cni = use_cni;
+       strlcpy(internals->sock_path, sock_path, PATH_MAX);

        if (xdp_get_channels_info(if_name, &internals->max_queue_cnt,
                                  &internals->combined_queue_cnt)) { @@ -
2328,6 +2337,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device *dev)
        int busy_budget = -1, ret;
        int force_copy = 0;
        int use_cni = 0;
+       char sock_path[PATH_MAX] = {'\0'};
        struct rte_eth_dev *eth_dev = NULL;
        const char *name = rte_vdev_device_name(dev);

@@ -2370,7 +2380,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
*dev)

        if (parse_parameters(kvlist, if_name, &xsk_start_queue_idx,
                             &xsk_queue_cnt, &shared_umem, prog_path,
-                            &busy_budget, &force_copy, &use_cni) < 0) {
+                            &busy_budget, &force_copy, &use_cni,
sock_path) < 0) {
                AF_XDP_LOG(ERR, "Invalid kvargs value\n");
                return -EINVAL;
        }
@@ -2387,6 +2397,13 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
*dev)
                        return -EINVAL;
        }

+       if (use_cni && !strnlen(sock_path, PATH_MAX)) {
+               AF_XDP_LOG(ERR, "When '%s' parameter is used, '%s' must
also be provided\n",
+                       ETH_AF_XDP_USE_CNI_ARG,
ETH_AF_XDP_SOCK_ARG);
+                       return -EINVAL;
+       }
+
+
        if (strlen(if_name) == 0) {
                AF_XDP_LOG(ERR, "Network interface must be
specified\n");
                return -EINVAL;
@@ -2410,7 +2427,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
*dev)

        eth_dev = init_internals(dev, if_name, xsk_start_queue_idx,
                                 xsk_queue_cnt, shared_umem, prog_path,
-                                busy_budget, force_copy, use_cni);
+                                busy_budget, force_copy, use_cni,
sock_path);
        if (eth_dev == NULL) {
                AF_XDP_LOG(ERR, "Failed to init internals\n");
                return -1;
@@ -2471,4 +2488,5 @@
RTE_PMD_REGISTER_PARAM_STRING(net_af_xdp,
                              "xdp_prog=<string> "
                              "busy_budget=<int> "
                              "force_copy=<int> "
-                             "use_cni=<int> ");
+                             "use_cni=<int> "
+                             "sock=<string> ");
--
2.41.0


Reply via email to