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 59699A0C47; Tue, 5 Oct 2021 14:41:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ED9DD4134A; Tue, 5 Oct 2021 14:41:43 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3CAC341344 for ; Tue, 5 Oct 2021 14:41:42 +0200 (CEST) Received: from [100.65.5.102] (unknown [5.144.121.98]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id A25E17F508; Tue, 5 Oct 2021 15:41:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A25E17F508 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633437701; bh=XwUencKTEhI3Dz/DYOycWFzuaXDMqZ2uWePbu2okK+8=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=nARSYyNL8ggsMlc1QhX6EHYZuFUsofqozINSZXhEP1muohxmGGQK+n66MBfXK7kj/ 4NFMoMAKLW+rTPILkD+ZIfkBL/9vEdjGgJgJzHJL9VPcRgHAQzBKeKDGcrvN33N7aj xN5jwgKsOWNCwcKXs+VZrmBtReY37h6cPfv5s9X8= To: Ori Kam , "dev@dpdk.org" Cc: Andrew Rybchenko , Xiaoyun Li , NBU-Contact-Thomas Monjalon , Ferruh Yigit , Ray Kinsella References: <20210907125157.3843-1-ivan.malov@oktetlabs.ru> <20211005003604.31684-1-ivan.malov@oktetlabs.ru> From: Ivan Malov Message-ID: <1d718593-bb63-4ab2-54ee-7b80540c4131@oktetlabs.ru> Date: Tue, 5 Oct 2021 15:41:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev 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 Sender: "dev" Hi Ori, Thanks a lot for eagerly reviewing this and related patches. That's very helpful of yours. On 05/10/2021 12:22, Ori Kam wrote: > Hi Ivan, > >> -----Original Message----- >> From: Ivan Malov >> Sent: Tuesday, October 5, 2021 3:36 AM >> Subject: [PATCH] ethdev: let apps find transfer admin port for a given ethdev >> >> Introduce a helper API to let applications find transfer admin port for a given >> ethdev to avoid failures when managing "transfer" flows through unprivileged >> ports. >> >> Signed-off-by: Ivan Malov >> Reviewed-by: Andrew Rybchenko >> --- >> Patch series [1] has reworked support for "transfer" flows. >> As a result, an application no longer needs to communicate such flows through >> a particular ethdev where it receives the corresponding packets in the first >> place. >> >> Hence, this patch is a legitimate follow-up to the series [1]. >> At the same time, it's a follow-up to the early RFC [2]. >> >> net/sfc driver is going to support this method. The corresponding patch is >> already in progress and will be provided in the course of this release cycle. >> >> [1] https://patches.dpdk.org/project/dpdk/list/?series=19326 >> [2] https://patches.dpdk.org/project/dpdk/list/?series=18737 >> --- >> app/test-pmd/config.c | 106 ++++++++++++++++++++++++- >> app/test-pmd/testpmd.c | 21 +++++ >> app/test-pmd/testpmd.h | 4 + >> app/test-pmd/util.c | 5 +- >> doc/guides/rel_notes/release_21_11.rst | 3 + >> lib/ethdev/rte_flow.c | 22 +++++ >> lib/ethdev/rte_flow.h | 32 ++++++++ >> lib/ethdev/rte_flow_driver.h | 5 ++ >> lib/ethdev/version.map | 3 + >> 9 files changed, 197 insertions(+), 4 deletions(-) >> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index >> 9c66329e96..2772c83d0a 100644 >> --- a/app/test-pmd/config.c >> +++ b/app/test-pmd/config.c >> @@ -1505,10 +1505,25 @@ port_action_handle_create(portid_t port_id, >> uint32_t id, >> struct port_indirect_action *pia; >> int ret; >> struct rte_flow_error error; >> + struct rte_port *port; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> > > Is this part of the patch or a general fix? I know it might seem unrelated at first glance. But it has to be added here for consistency with the rest of the code being added. It should be OK. > >> ret = action_alloc(port_id, id, &pia); >> if (ret) >> return ret; >> + >> + port = &ports[port_id]; >> + >> + if (conf->transfer) >> + port_id = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> if (action->type == RTE_FLOW_ACTION_TYPE_AGE) { >> struct rte_flow_action_age *age = >> (struct rte_flow_action_age *)(uintptr_t)(action->conf); >> @@ -1531,6 +1546,7 @@ port_action_handle_create(portid_t port_id, >> uint32_t id, >> return port_flow_complain(&error); >> } >> pia->type = action->type; >> + pia->transfer = conf->transfer; >> printf("Indirect action #%u created\n", pia->id); >> return 0; >> } >> @@ -1557,9 +1573,18 @@ port_action_handle_destroy(portid_t port_id, >> for (i = 0; i != n; ++i) { >> struct rte_flow_error error; >> struct port_indirect_action *pia = *tmp; >> + portid_t port_id_eff = port_id; >> >> if (actions[i] != pia->id) >> continue; >> + >> + if (pia->transfer) >> + port_id_eff = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id_eff, ENABLED_WARN) || >> + port_id_eff == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> /* >> * Poisoning to make sure PMDs update it in case >> * of error. >> @@ -1567,7 +1592,7 @@ port_action_handle_destroy(portid_t port_id, >> memset(&error, 0x33, sizeof(error)); >> >> if (pia->handle && rte_flow_action_handle_destroy( >> - port_id, pia->handle, &error)) { >> + port_id_eff, pia->handle, &error)) { >> ret = port_flow_complain(&error); >> continue; >> } >> @@ -1602,8 +1627,15 @@ port_action_handle_update(portid_t port_id, >> uint32_t id, >> struct rte_flow_error error; >> struct rte_flow_action_handle *action_handle; >> struct port_indirect_action *pia; >> + struct rte_port *port; >> const void *update; >> >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> + port = &ports[port_id]; >> + >> action_handle = port_action_handle_get_by_id(port_id, id); >> if (!action_handle) >> return -EINVAL; >> @@ -1618,6 +1650,14 @@ port_action_handle_update(portid_t port_id, >> uint32_t id, >> update = action; >> break; >> } >> + >> + if (pia->transfer) >> + port_id = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> if (rte_flow_action_handle_update(port_id, action_handle, update, >> &error)) { >> return port_flow_complain(&error); >> @@ -1636,6 +1676,14 @@ port_action_handle_query(portid_t port_id, >> uint32_t id) >> struct rte_flow_query_age age; >> struct rte_flow_action_conntrack ct; >> } query; >> + portid_t port_id_eff = port_id; >> + struct rte_port *port; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> + port = &ports[port_id]; >> >> pia = action_get_by_id(port_id, id); >> if (!pia) >> @@ -1650,10 +1698,19 @@ port_action_handle_query(portid_t port_id, >> uint32_t id) >> id, pia->type, port_id); >> return -ENOTSUP; >> } >> + >> + if (pia->transfer) >> + port_id_eff = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id_eff, ENABLED_WARN) || >> + port_id_eff == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> /* Poisoning to make sure PMDs update it in case of error. */ >> memset(&error, 0x55, sizeof(error)); >> memset(&query, 0, sizeof(query)); >> - if (rte_flow_action_handle_query(port_id, pia->handle, &query, >> &error)) >> + if (rte_flow_action_handle_query(port_id_eff, pia->handle, &query, >> + &error)) >> return port_flow_complain(&error); >> switch (pia->type) { >> case RTE_FLOW_ACTION_TYPE_AGE: >> @@ -1872,6 +1929,20 @@ port_flow_validate(portid_t port_id, { >> struct rte_flow_error error; >> struct port_flow_tunnel *pft = NULL; >> + struct rte_port *port; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> + port = &ports[port_id]; >> + >> + if (attr->transfer) >> + port_id = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> >> /* Poisoning to make sure PMDs update it in case of error. */ >> memset(&error, 0x11, sizeof(error)); >> @@ -1925,7 +1996,19 @@ port_flow_create(portid_t port_id, >> struct port_flow_tunnel *pft = NULL; >> struct rte_flow_action_age *age = age_action_get(actions); >> >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> port = &ports[port_id]; >> + >> + if (attr->transfer) >> + port_id = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> if (port->flow_list) { >> if (port->flow_list->id == UINT32_MAX) { >> fprintf(stderr, >> @@ -1989,6 +2072,7 @@ port_flow_destroy(portid_t port_id, uint32_t n, const >> uint32_t *rule) >> uint32_t i; >> >> for (i = 0; i != n; ++i) { >> + portid_t port_id_eff = port_id; >> struct rte_flow_error error; >> struct port_flow *pf = *tmp; >> >> @@ -1999,7 +2083,15 @@ port_flow_destroy(portid_t port_id, uint32_t n, >> const uint32_t *rule) >> * of error. >> */ >> memset(&error, 0x33, sizeof(error)); >> - if (rte_flow_destroy(port_id, pf->flow, &error)) { >> + >> + if (pf->rule.attr->transfer) >> + port_id_eff = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id_eff, ENABLED_WARN) || >> + port_id_eff == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> + if (rte_flow_destroy(port_id_eff, pf->flow, &error)) { >> ret = port_flow_complain(&error); >> continue; >> } >> @@ -2133,6 +2225,14 @@ port_flow_query(portid_t port_id, uint32_t rule, >> fprintf(stderr, "Flow rule #%u not found\n", rule); >> return -ENOENT; >> } >> + >> + if (pf->rule.attr->transfer) >> + port_id = port->flow_transfer_proxy; >> + >> + if (port_id_is_invalid(port_id, ENABLED_WARN) || >> + port_id == (portid_t)RTE_PORT_ALL) >> + return -EINVAL; >> + >> ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR, >> &name, sizeof(name), >> (void *)(uintptr_t)action->type, &error); diff --git >> a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index >> 97ae52e17e..a88e920bd0 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c >> @@ -533,6 +533,25 @@ int proc_id; >> */ >> unsigned int num_procs = 1; >> >> +static void >> +flow_pick_transfer_proxy_mp(uint16_t port_id) { >> + struct rte_port *port = &ports[port_id]; >> + int ret; >> + >> + port->flow_transfer_proxy = port_id; >> + >> + if (!is_proc_primary()) >> + return; >> + >> + ret = rte_flow_pick_transfer_proxy(port_id, &port- >>> flow_transfer_proxy, >> + NULL); >> + if (ret != 0) { >> + fprintf(stderr, "Error picking flow transfer proxy for port %u: %s >> - ignore\n", >> + port_id, rte_strerror(-ret)); >> + } >> +} >> + >> static int >> eth_dev_configure_mp(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> const struct rte_eth_conf *dev_conf) @@ -1489,6 +1508,8 >> @@ init_config_port_offloads(portid_t pid, uint32_t socket_id) >> int ret; >> int i; >> >> + flow_pick_transfer_proxy_mp(pid); >> + >> port->dev_conf.txmode = tx_mode; >> port->dev_conf.rxmode = rx_mode; >> >> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index >> 5863b2f43f..b3dfd350e5 100644 >> --- a/app/test-pmd/testpmd.h >> +++ b/app/test-pmd/testpmd.h >> @@ -173,6 +173,8 @@ struct port_indirect_action { >> enum rte_flow_action_type type; /**< Action type. */ >> struct rte_flow_action_handle *handle; /**< Indirect action handle. */ >> enum age_action_context_type age_type; /**< Age action context >> type. */ >> + /** If true, the action applies to "transfer" flows, and vice versa */ >> + bool transfer; >> }; >> >> struct port_flow_tunnel { >> @@ -234,6 +236,8 @@ struct rte_port { >> /**< dynamic flags. */ >> uint64_t mbuf_dynf; >> const struct rte_eth_rxtx_callback >> *tx_set_dynf_cb[RTE_MAX_QUEUES_PER_PORT+1]; >> + /** Associated port which is supposed to handle "transfer" flows */ >> + portid_t flow_transfer_proxy; >> }; >> >> /** >> diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c index >> 14a9a251fb..d9edbbf9ee 100644 >> --- a/app/test-pmd/util.c >> +++ b/app/test-pmd/util.c >> @@ -98,13 +98,16 @@ dump_pkt_burst(uint16_t port_id, uint16_t queue, >> struct rte_mbuf *pkts[], >> int ret; >> struct rte_flow_error error; >> struct rte_flow_restore_info info = { 0, }; >> + struct rte_port *port = &ports[port_id]; >> >> mb = pkts[i]; >> eth_hdr = rte_pktmbuf_read(mb, 0, sizeof(_eth_hdr), >> &_eth_hdr); >> eth_type = RTE_BE_TO_CPU_16(eth_hdr->ether_type); >> packet_type = mb->packet_type; >> is_encapsulation = RTE_ETH_IS_TUNNEL_PKT(packet_type); >> - ret = rte_flow_get_restore_info(port_id, mb, &info, &error); >> + >> + ret = rte_flow_get_restore_info(port->flow_transfer_proxy, >> + mb, &info, &error); > > I'm not sure this is correct, > Since to restore the data you need to know the port that this traffic was sent to. > Even if the action was offloaded on the proxy port, it is important to know what was the > destination port. > even setting the tunnel must be done on the target port and not the proxy port. Is tunnel offload a "transfer"-based feature? My answer is "yes". And, if we all agree that it's easier to communicate all "transfer"-related requests through a single ("transfer" admin) ethdev, then the first argument in the highlighted invocation also should be this "proxy" ethdev. A unified entry point. But please don't think me forward: I fully understand your concern about the real DPDK port through which the packet was delivered to the application. But that should not be a big problem. The mbuf which is passed to this API still has its metadata set (tunnel mark, for instance) which the PMD can use to recall the port which this packet was sent to. More to that, the mbuf even has a dedicated field to express exactly what you may want to have - the "port" field. So, when your PMD receives the "missed" packet and hands it over to the application through some ethdev / port, it can set this mbuf fields to that port ID. This way, such information is not lost because of migration to "transfer proxy" solution, is it? > >> if (!ret) { >> MKDUMPSTR(print_buf, buf_size, cur_len, >> "restore info:"); >> diff --git a/doc/guides/rel_notes/release_21_11.rst >> b/doc/guides/rel_notes/release_21_11.rst >> index 37776c135e..cc0ea4695a 100644 >> --- a/doc/guides/rel_notes/release_21_11.rst >> +++ b/doc/guides/rel_notes/release_21_11.rst >> @@ -67,6 +67,9 @@ New Features >> Added macros ETH_RSS_IPV4_CHKSUM and ETH_RSS_L4_CHKSUM, now IPv4 >> and >> TCP/UDP/SCTP header checksum field can be used as input set for RSS. >> >> +* **Added an API to pick flow transfer proxy port** >> + A new API, ``rte_flow_pick_transfer_proxy()``, was added. >> + >> * **Updated Broadcom bnxt PMD.** >> >> * Added flow offload support for Thor. >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index >> 647bbf91ce..15e978f7f7 100644 >> --- a/lib/ethdev/rte_flow.c >> +++ b/lib/ethdev/rte_flow.c >> @@ -1270,3 +1270,25 @@ rte_flow_tunnel_item_release(uint16_t port_id, >> RTE_FLOW_ERROR_TYPE_UNSPECIFIED, >> NULL, rte_strerror(ENOTSUP)); >> } >> + >> +int >> +rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id, >> + struct rte_flow_error *error) > > What about replaceing pick with get? There's a school of thought that terms like "get" imply the existence of paired actions. For example, "get - set", "probe - unprobe", "allocate - free", "init - fini". So, in order to avoid any wrong assumptions, I believe it's better to stick with term "pick" here as it doesn't seem to have a paired term. > >> +{ >> + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; >> + const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); >> + >> + if (unlikely(ops == NULL)) >> + return -rte_errno; >> + >> + if (likely(ops->pick_transfer_proxy != NULL)) { >> + return flow_err(port_id, >> + ops->pick_transfer_proxy(dev, proxy_port_id, >> + error), >> + error); >> + } >> + >> + *proxy_port_id = port_id; >> + >> + return 0; >> +} >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index >> f195aa7224..d2cb476189 100644 >> --- a/lib/ethdev/rte_flow.h >> +++ b/lib/ethdev/rte_flow.h >> @@ -122,6 +122,10 @@ struct rte_flow_attr { >> * >> * In order to match traffic originating from specific source(s), the >> * application should use pattern items ETHDEV and ESWITCH_PORT. >> + * >> + * Communicating "transfer" flows via unprivileged ethdevs may not >> + * be possible. In order to pick the ethdev suitable for that, the >> + * application should use @p rte_flow_pick_transfer_proxy(). >> */ >> uint32_t transfer:1; >> uint32_t reserved:29; /**< Reserved, must be zero. */ @@ -4427,6 >> +4431,34 @@ rte_flow_tunnel_item_release(uint16_t port_id, >> struct rte_flow_item *items, >> uint32_t num_of_items, >> struct rte_flow_error *error); >> + >> +/** >> + * An application receiving packets on a given ethdev may want to have >> +their >> + * processing offloaded to the e-switch lying beneath this ethdev by >> +means >> + * of maintaining "transfer" flows. However, it need never use this >> +exact >> + * ethdev as an entry point for such flows to be managed through. More >> +to >> + * that, this particular ethdev may be short of privileges to control >> +the >> + * e-switch. Instead, the application should find an admin ethdev >> +sitting >> + * on top of the same e-switch to be used as the entry point (a "proxy"). >> + * > > This explanation is not clear, can you rephrase it? Could you please be so kind to outline the parts which are not clear to you. Maybe your understanding is right in fact but you are just unsure. If you expressed your interpretation of this text, I could review it and either confirm its correctness or debunk any misunderstanding. In the latter case, it would be easier to me to rephrase the comment in the patch. > >> + * This API is a helper to find such "transfer proxy" for a given ethdev. >> + * >> + * @note >> + * If the PMD serving @p port_id doesn't have the corresponding method >> + * implemented, the API will return @p port_id via @p proxy_port_id. > > +1 > >> + * >> + * @param port_id >> + * ID of the ethdev in question >> + * @param[out] proxy_port_id >> + * ID of the "transfer proxy" >> + * >> + * @return >> + * 0 on success, a negative error code otherwise >> + */ >> +__rte_experimental >> +int >> +rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id, >> + struct rte_flow_error *error); >> #ifdef __cplusplus >> } >> #endif >> diff --git a/lib/ethdev/rte_flow_driver.h b/lib/ethdev/rte_flow_driver.h index >> 46f62c2ec2..ed52e59a0a 100644 >> --- a/lib/ethdev/rte_flow_driver.h >> +++ b/lib/ethdev/rte_flow_driver.h >> @@ -139,6 +139,11 @@ struct rte_flow_ops { >> struct rte_flow_item *pmd_items, >> uint32_t num_of_items, >> struct rte_flow_error *err); >> + /** See rte_flow_pick_transfer_proxy() */ >> + int (*pick_transfer_proxy) >> + (struct rte_eth_dev *dev, >> + uint16_t *proxy_port_id, >> + struct rte_flow_error *error); >> }; >> >> /** >> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index >> 904bce6ea1..d4286dc8dd 100644 >> --- a/lib/ethdev/version.map >> +++ b/lib/ethdev/version.map >> @@ -247,6 +247,9 @@ EXPERIMENTAL { >> rte_mtr_meter_policy_delete; >> rte_mtr_meter_policy_update; >> rte_mtr_meter_policy_validate; >> + >> + # added in 21.11 >> + rte_flow_pick_transfer_proxy; >> }; >> >> INTERNAL { >> -- >> 2.20.1 > > Best, > Ori > -- Ivan M