From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C0E3243410; Thu, 30 Nov 2023 13:33:05 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 94BD842EDB; Thu, 30 Nov 2023 13:33:05 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 06A2942E97 for ; Thu, 30 Nov 2023 13:33:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1701347583; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Lte5s4IQhHIY//BPj2Mmru+U0cFvcmhSZVCwrI6f8NE=; b=VFSto1uP8EcnaQkups00kvjPJBmFayt2EKljCI7E6yaMcjeJgFoYhl4mlQX8Cta5lYCz/C jG7x5TMVUiMHuR1if6FobvUa55Yr+zljeBpnX1MAlP+aNnxCYX/hFC55uE5tS5HOhn97OC /J/9P9u5ZseLPiogkL5prif7xlTfaj4= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-464-XyB8a3OhPLKv9MYP81qspA-1; Thu, 30 Nov 2023 07:33:01 -0500 X-MC-Unique: XyB8a3OhPLKv9MYP81qspA-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-40b443d698eso6414505e9.2 for ; Thu, 30 Nov 2023 04:33:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701347580; x=1701952380; h=in-reply-to:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=Lte5s4IQhHIY//BPj2Mmru+U0cFvcmhSZVCwrI6f8NE=; b=qWoQ6Ibb353DG+JkKlc2HVuIdWjTn+chTQU8zWQHvWjg7BUwWc7eb5CxIFYoMMsywY NqrjJ7aYdJ0oaPMfO4KL8dT2JX/HRsNpw/SFoMHPc/8ubntR/MPPqgbAVppvW7i7DP+O pLYJ3ZOBA5HvUi6fW8SprQonTopHtiXSJ+7M5Me/B6XOMvIuiahNxoTrkFCKYe8xYpQ7 Hr9lVx7R/MgsCiNtGvP19Ay2Yt/LYqhSPQ7pyHoaqrlYXAuZKzzXGMiu8I3rZLhvRx6f Czt1St69dgi1PemJHRmJbux2r1cv9ZgjpiCGLAMiOpIhnRthFus+dfvAD7+FdHnw38ll 4l6A== X-Gm-Message-State: AOJu0YwpkVaj3WOg7rSxn8atsipOeCkbZYCs4Xo6cS7fqwRmKJVa22yg +meQdUG3oCic94KsYDk7z9zV3lEHY9vVjR9U7zDUaJSswjrHOXAYWdL8vG+Obr9ZSDEfXrI0dun yfG4= X-Received: by 2002:a05:600c:474d:b0:40b:3802:6ef8 with SMTP id w13-20020a05600c474d00b0040b38026ef8mr15247526wmo.34.1701347580313; Thu, 30 Nov 2023 04:33:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IGGGfn6EVDEmEfQYRdrKMfSvalQj4ct2V0iV9zNBjkyBVFjQ905+Fide/7ry1G1/Mkw/j8XlQ== X-Received: by 2002:a05:600c:474d:b0:40b:3802:6ef8 with SMTP id w13-20020a05600c474d00b0040b38026ef8mr15247509wmo.34.1701347579884; Thu, 30 Nov 2023 04:32:59 -0800 (PST) Received: from [192.168.0.12] ([78.18.22.228]) by smtp.gmail.com with ESMTPSA id h19-20020a05600c351300b0040b347d90d0sm5424095wmq.12.2023.11.30.04.32.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 30 Nov 2023 04:32:59 -0800 (PST) Message-ID: Date: Thu, 30 Nov 2023 12:32:58 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [v1] net/af_xdp: enable a sock path alongside use_cni To: "Koikkara Reeny, Shibin" , "ferruh.yigit@amd.com" , "stephen@networkplumber.org" , "lihuisong@huawei.com" , "fengchengwen@huawei.com" , "liuyonglong@huawei.com" Cc: "dev@dpdk.org" References: <20231130091332.2315572-1-mtahhan@redhat.com> From: Maryam Tahhan In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="------------AZCkvEMsHLdoxnEOaIkTLWcy" Content-Language: en-US X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org This is a multi-part message in MIME format. --------------AZCkvEMsHLdoxnEOaIkTLWcy Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 insidemake_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. > >> 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? > >> + 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= " >> "busy_budget= " >> "force_copy= " >> - "use_cni= "); >> + "use_cni= " >> + "sock= "); >> -- >> 2.41.0 --------------AZCkvEMsHLdoxnEOaIkTLWcy Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit
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.



 		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?




+	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


    

    


    

  


--------------AZCkvEMsHLdoxnEOaIkTLWcy--