Hi Maryam, I have added some suggestion below.
Regrads, Shibin > -----Original Message----- > From: Maryam Tahhan <[email protected]> > Sent: Thursday, November 30, 2023 9:14 AM > To: [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected] > Cc: [email protected]; Tahhan, Maryam <[email protected]> > 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 <[email protected]> > --- > 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 > 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
