Hi Maryam,

I have replied inline.

Regards,

Shibin

From: Maryam Tahhan <mtahhan@redhat.com>
Sent: Thursday, November 30, 2023 12:33 PM
To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>; ferruh.yigit@amd.com; stephen@networkplumber.org; lihuisong@huawei.com; fengchengwen@huawei.com; liuyonglong@huawei.com
Cc: dev@dpdk.org
Subject: Re: [v1] net/af_xdp: enable a sock path alongside use_cni

Hi Shibin

Thanks for the review. Comments inline.

BR
Maryam

On 30/11/2023 12:14, Koikkara Reeny, Shibin wrote:

Hi Maryam,
 
I have added some suggestion below.
 
Regrads,
Shibin

[snip]

 
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".

No issues in renaming

  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

No issues in renaming

.
 

 
 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()?

it's used in the send_msg() function inside make_request_cni(). In the original implementation of send_msg used a #defined value which is why it wasn't needed before. so as far as I can tell send_msg still needs that path. but I'm open to suggestions.

Sorry my mistake. You will need to pass the argument.

          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.

Please see previous ans

          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.

Please see previous ans

          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.

Please see previous ans

          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 there's an issue here processing is abandoned. Is it not better to catch any issue early on? I'm not sure I understand the race condition?

Race condition if af_xdp application pod start by a bit of out of sync before the USD server thread start. So we can wait till the initialization process. It is a suggestion.

+ 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