From: Maryam Tahhan <mtahhan@redhat.com>
To: "Loftus, Ciara" <ciara.loftus@intel.com>,
"ferruh.yigit@amd.com" <ferruh.yigit@amd.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>,
"lihuisong@huawei.com" <lihuisong@huawei.com>,
"fengchengwen@huawei.com" <fengchengwen@huawei.com>,
"liuyonglong@huawei.com" <liuyonglong@huawei.com>,
"Koikkara Reeny, Shibin" <shibin.koikkara.reeny@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [v4] net/af_xdp: enable uds_path instead of use_cni
Date: Fri, 15 Dec 2023 10:26:13 +0000 [thread overview]
Message-ID: <b736e7ae-a3ea-4fa8-884e-77548961766d@redhat.com> (raw)
In-Reply-To: <MW4PR11MB587215552A95C068FE63D32D8E93A@MW4PR11MB5872.namprd11.prod.outlook.com>
Thanks Ciara, I missed the af_xdp.rst :(
Will update and push a v5 shortly
On 15/12/2023 09:55, Loftus, Ciara wrote:
> Thanks for the latest patch Maryam. I have one minor suggestion inline.
> Also, there are still some references to "use_cni" in af_xdp.rst which should be removed/replaced with uds_path.
> Once that's done I think the patch should be good to go. Perhaps also consider adding a note to the release notes mentioning the new functionality.
>
> Thanks,
> Ciara
>
>> With the original 'use_cni' implementation, (using a
>> hardcoded socket rather than a configurable one),
>> if a DPDK 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 using a new vdev param called
>> 'uds_path' and removing the previous 'use_cni' param.
>> This patch also fixes incorrect references to the
>> AF_XDP DP as CNI and updates the documentation with a
>> working example. This change has been tested with the
>> AF_XDP DP PR 81, with both single and multiple interfaces.
>>
>> v4:
>> * Rename af_xdp_cni.rst to af_xdp_dp.rst
>> * Removed all incorrect references to CNI throughout af_xdp
>> PMD file.
>> * Fixed Typos in af_xdp_dp.rst
>>
>> v3:
>> * Remove `use_cni` vdev argument as it's no longer needed.
>> * Update incorrect CNI references for the AF_XDP DP in the
>> documentation.
>> * Update the documentation to run a simple example with the
>> AF_XDP DP plugin in K8s.
>>
>> v2:
>> * Rename sock_path to uds_path.
>> * Update documentation to reflect when CAP_BPF is needed.
>> * Fix testpmd arguments in the provided example for Pods.
>> * Use AF_XDP API to update the xskmap entry.
>>
>> Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
>> ---
>> doc/guides/howto/af_xdp_cni.rst | 253 -------------------------
>> doc/guides/howto/af_xdp_dp.rst | 278
>> ++++++++++++++++++++++++++++
>> doc/guides/howto/index.rst | 2 +-
>> drivers/net/af_xdp/rte_eth_af_xdp.c | 100 +++++-----
>> 4 files changed, 328 insertions(+), 305 deletions(-)
>> delete mode 100644 doc/guides/howto/af_xdp_cni.rst
>> create mode 100644 doc/guides/howto/af_xdp_dp.rst
>>
> <snip>
>
>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>> index 353c8688ec..6caad58e60 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"
>> @@ -170,7 +169,7 @@ struct pmd_internals {
>> char prog_path[PATH_MAX];
>> bool custom_prog_configured;
>> bool force_copy;
>> - bool use_cni;
>> + char uds_path[PATH_MAX];
>> struct bpf_map *map;
>>
>> struct rte_ether_addr eth_addr;
>> @@ -190,7 +189,7 @@ struct pmd_process_private {
>> #define ETH_AF_XDP_PROG_ARG "xdp_prog"
>> #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_USE_DP_UDS_PATH_ARG "uds_path"
> Use the same alignment for "uds_path" as the strings above it.
>
>> static const char * const valid_arguments[] = {
>> ETH_AF_XDP_IFACE_ARG,
>> @@ -200,7 +199,7 @@ static const char * const valid_arguments[] = {
>> ETH_AF_XDP_PROG_ARG,
>> ETH_AF_XDP_BUDGET_ARG,
>> ETH_AF_XDP_FORCE_COPY_ARG,
>> - ETH_AF_XDP_USE_CNI_ARG,
>> + ETH_AF_XDP_USE_DP_UDS_PATH_ARG,
>> NULL
>> };
>>
>> @@ -1351,7 +1350,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 *uds_path)
>> {
>> int sock;
>>
>> @@ -1362,7 +1361,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, uds_path, sizeof(server->sun_path));
>>
>> if (connect(sock, (struct sockaddr *)server, sizeof(struct
>> sockaddr_un)) < 0) {
>> close(sock);
>> @@ -1382,7 +1381,7 @@ struct msg_internal {
>> };
>>
>> static int
>> -send_msg(int sock, char *request, int *fd)
>> +send_msg(int sock, char *request, int *fd, const char *uds_path)
>> {
>> int snd;
>> struct iovec iov;
>> @@ -1393,7 +1392,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, uds_path, sizeof(dst.sun_path));
>>
>> /* Initialize message header structure */
>> memset(&msgh, 0, sizeof(msgh));
>> @@ -1470,8 +1469,8 @@ 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)
>> +make_request_dp(int sock, struct sockaddr_un *server, char *request,
>> + int *req_fd, char *response, int *out_fd, const char
>> *uds_path)
>> {
>> int rval;
>>
>> @@ -1483,7 +1482,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, uds_path);
>>
>> if (rval < 0) {
>> AF_XDP_LOG(ERR, "Write error %s\n", strerror(errno));
>> @@ -1507,7 +1506,7 @@ check_response(char *response, char *exp_resp,
>> long size)
>> }
>>
>> static int
>> -get_cni_fd(char *if_name)
>> +get_xskmap_fd(char *if_name, const char *uds_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 +1519,14 @@ get_cni_fd(char *if_name)
>> return -1;
>>
>> memset(&server, 0, sizeof(server));
>> - sock = init_uds_sock(&server);
>> + sock = init_uds_sock(&server, uds_path);
>> if (sock < 0)
>> return -1;
>>
>> - /* Initiates handshake to CNI send: /connect,hostname */
>> + /* Initiates handshake to AF_XDP Device Plugin 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_dp(sock, &server, request, NULL, response,
>> &out_fd, uds_path) < 0) {
>> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", request);
>> goto err_close;
>> }
>> @@ -1541,7 +1540,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_dp(sock, &server, request, NULL, response,
>> &out_fd, uds_path) < 0) {
>> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", request);
>> goto err_close;
>> }
>> @@ -1549,7 +1548,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_dp(sock, &server, request, NULL, response,
>> &out_fd, uds_path) < 0) {
>> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", request);
>> goto err_close;
>> }
>> @@ -1571,7 +1570,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_dp(sock, &server, request, NULL, response,
>> &out_fd, uds_path) < 0) {
>> AF_XDP_LOG(ERR, "Error in processing cmd [%s]\n", request);
>> goto err_close;
>> }
>> @@ -1640,7 +1639,7 @@ xsk_configure(struct pmd_internals *internals,
>> struct pkt_rx_queue *rxq,
>> #endif
>>
>> /* Disable libbpf from loading XDP program */
>> - if (internals->use_cni)
>> + if (strnlen(internals->uds_path, PATH_MAX))
>> cfg.libbpf_flags |= XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD;
>>
>> if (strnlen(internals->prog_path, PATH_MAX)) {
>> @@ -1694,18 +1693,17 @@ xsk_configure(struct pmd_internals *internals,
>> struct pkt_rx_queue *rxq,
>> }
>> }
>>
>> - if (internals->use_cni) {
>> - int err, fd, map_fd;
>> + if (strnlen(internals->uds_path, PATH_MAX)) {
>> + int err, map_fd;
>>
>> - /* get socket fd from CNI plugin */
>> - map_fd = get_cni_fd(internals->if_name);
>> + /* get socket fd from AF_XDP Device plugin */
>> + map_fd = get_xskmap_fd(internals->if_name, internals-
>>> uds_path);
>> if (map_fd < 0) {
>> - AF_XDP_LOG(ERR, "Failed to receive CNI plugin fd\n");
>> + AF_XDP_LOG(ERR, "Failed to receive AF_XDP Device
>> plugin fd\n");
>> goto out_xsk;
>> }
>> - /* get socket fd */
>> - fd = xsk_socket__fd(rxq->xsk);
>> - err = bpf_map_update_elem(map_fd, &rxq->xsk_queue_idx,
>> &fd, 0);
>> +
>> + err = xsk_socket__update_xskmap(rxq->xsk, map_fd);
>> if (err) {
>> AF_XDP_LOG(ERR, "Failed to insert unprivileged xsk in
>> map.\n");
>> goto out_xsk;
>> @@ -1881,13 +1879,13 @@ static const struct eth_dev_ops ops = {
>> .get_monitor_addr = eth_get_monitor_addr,
>> };
>>
>> -/* CNI option works in unprivileged container environment
>> - * and ethernet device functionality will be reduced. So
>> - * additional customiszed eth_dev_ops struct is needed
>> - * for cni. Promiscuous enable and disable functionality
>> - * is removed.
>> - **/
>> -static const struct eth_dev_ops ops_cni = {
>> +/* AF_XDP Device Plugin option works in unprivileged
>> + * container environment and ethernet device functionality
>> + * will be reduced. So additional customized eth_dev_ops
>> + * struct is needed for the AF_XDP Device Plugin. Promiscuous
>> + * enable and disable functionality is removed.
>> + */
>> +static const struct eth_dev_ops ops_afxdp_dp = {
>> .dev_start = eth_dev_start,
>> .dev_stop = eth_dev_stop,
>> .dev_close = eth_dev_close,
>> @@ -1957,7 +1955,7 @@ parse_name_arg(const char *key __rte_unused,
>>
>> /** parse xdp prog argument */
>> static int
>> -parse_prog_arg(const char *key __rte_unused,
>> +parse_path_arg(const char *key __rte_unused,
>> const char *value, void *extra_args)
>> {
>> char *path = extra_args;
>> @@ -2023,7 +2021,7 @@ 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, char *uds_path)
>> {
>> int ret;
>>
>> @@ -2050,7 +2048,7 @@ parse_parameters(struct rte_kvargs *kvlist, char
>> *if_name, int *start_queue,
>> goto free_kvlist;
>>
>> ret = rte_kvargs_process(kvlist, ETH_AF_XDP_PROG_ARG,
>> - &parse_prog_arg, prog_path);
>> + &parse_path_arg, prog_path);
>> if (ret < 0)
>> goto free_kvlist;
>>
>> @@ -2064,8 +2062,8 @@ 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_USE_CNI_ARG,
>> - &parse_integer_arg, use_cni);
>> + ret = rte_kvargs_process(kvlist,
>> ETH_AF_XDP_USE_DP_UDS_PATH_ARG,
>> + &parse_path_arg, uds_path);
>> if (ret < 0)
>> goto free_kvlist;
>>
>> @@ -2108,7 +2106,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)
>> + const char *uds_path)
>> {
>> const char *name = rte_vdev_device_name(dev);
>> const unsigned int numa_node = dev->device.numa_node;
>> @@ -2137,7 +2135,7 @@ init_internals(struct rte_vdev_device *dev, const
>> char *if_name,
>> #endif
>> internals->shared_umem = shared_umem;
>> internals->force_copy = force_copy;
>> - internals->use_cni = use_cni;
>> + strlcpy(internals->uds_path, uds_path, PATH_MAX);
>>
>> if (xdp_get_channels_info(if_name, &internals->max_queue_cnt,
>> &internals->combined_queue_cnt)) {
>> @@ -2196,10 +2194,10 @@ init_internals(struct rte_vdev_device *dev, const
>> char *if_name,
>> eth_dev->data->dev_link = pmd_link;
>> eth_dev->data->mac_addrs = &internals->eth_addr;
>> eth_dev->data->dev_flags |=
>> RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
>> - if (!internals->use_cni)
>> + if (!strnlen(internals->uds_path, PATH_MAX))
>> eth_dev->dev_ops = &ops;
>> else
>> - eth_dev->dev_ops = &ops_cni;
>> + eth_dev->dev_ops = &ops_afxdp_dp;
>>
>> eth_dev->rx_pkt_burst = eth_af_xdp_rx;
>> eth_dev->tx_pkt_burst = eth_af_xdp_tx;
>> @@ -2327,7 +2325,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device
>> *dev)
>> char prog_path[PATH_MAX] = {'\0'};
>> int busy_budget = -1, ret;
>> int force_copy = 0;
>> - int use_cni = 0;
>> + char uds_path[PATH_MAX] = {'\0'};
>> struct rte_eth_dev *eth_dev = NULL;
>> const char *name = rte_vdev_device_name(dev);
>>
>> @@ -2370,20 +2368,20 @@ 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, uds_path) < 0) {
>> AF_XDP_LOG(ERR, "Invalid kvargs value\n");
>> return -EINVAL;
>> }
>>
>> - if (use_cni && busy_budget > 0) {
>> + if (strnlen(uds_path, PATH_MAX) && busy_budget > 0) {
>> AF_XDP_LOG(ERR, "When '%s' parameter is used, '%s'
>> parameter is not valid\n",
>> - ETH_AF_XDP_USE_CNI_ARG,
>> ETH_AF_XDP_BUDGET_ARG);
>> + ETH_AF_XDP_USE_DP_UDS_PATH_ARG,
>> ETH_AF_XDP_BUDGET_ARG);
>> return -EINVAL;
>> }
>>
>> - if (use_cni && strnlen(prog_path, PATH_MAX)) {
>> + if (strnlen(uds_path, PATH_MAX) && strnlen(prog_path, PATH_MAX))
>> {
>> AF_XDP_LOG(ERR, "When '%s' parameter is used, '%s'
>> parameter is not valid\n",
>> - ETH_AF_XDP_USE_CNI_ARG,
>> ETH_AF_XDP_PROG_ARG);
>> + ETH_AF_XDP_USE_DP_UDS_PATH_ARG,
>> ETH_AF_XDP_PROG_ARG);
>> return -EINVAL;
>> }
>>
>> @@ -2410,7 +2408,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, uds_path);
>> if (eth_dev == NULL) {
>> AF_XDP_LOG(ERR, "Failed to init internals\n");
>> return -1;
>> @@ -2471,4 +2469,4 @@ RTE_PMD_REGISTER_PARAM_STRING(net_af_xdp,
>> "xdp_prog=<string> "
>> "busy_budget=<int> "
>> "force_copy=<int> "
>> - "use_cni=<int> ");
>> + "uds_path=<string> ");
>> --
>> 2.41.0
prev parent reply other threads:[~2023-12-15 10:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-13 12:24 Maryam Tahhan
2023-12-15 9:55 ` Loftus, Ciara
2023-12-15 10:26 ` Maryam Tahhan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b736e7ae-a3ea-4fa8-884e-77548961766d@redhat.com \
--to=mtahhan@redhat.com \
--cc=ciara.loftus@intel.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=ferruh.yigit@amd.com \
--cc=lihuisong@huawei.com \
--cc=liuyonglong@huawei.com \
--cc=shibin.koikkara.reeny@intel.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).