DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev
@ 2021-10-06 13:23 Ivan Malov
  0 siblings, 0 replies; 8+ messages in thread
From: Ivan Malov @ 2021-10-06 13:23 UTC (permalink / raw)
  To: Thomas Monjalon, dev

Hi Thomas,

Thanks a lot for providing your review notes. So useful and to the 
point. I also processed review notes from Andrew and sent v3:

https://patches.dpdk.org/project/dpdk/patch/20211006123300.7848-1-ivan.malov@oktetlabs.ru/

I hope the new API is much clearer now. Should you wish to improve more 
aspects, you're welcome to point them out.

On 05/10/2021 22:04, Thomas Monjalon wrote:
 >05/10/2021 20:22, Ivan Malov:
 >> Hi Thomas,
 >>
 >> Thanks for joining the review.
 >>
 >> On 05/10/2021 20:56, Thomas Monjalon wrote:
 >> > 05/10/2021 02:36, Ivan Malov:
 >> >> 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.
 >> >
 >> > Please explain what is transfer admin port.
 >> > We may find a simpler wording.
 >>
 >> It's an ethdev which has the privilege to control the embedded switch.
 >> Typically, it's a PF ethdev.
 >>
 >> Flows with "transfer" attribute apply to the e-switch level. So
 >> "transfer admin port" reads the same as "e-switch admin port".
 >
 >Please explain this in the v2.
 >
 >> >> --- a/lib/ethdev/rte_flow.h
 >> >> +++ b/lib/ethdev/rte_flow.h
 >> >> +	 *
 >> >> +	 * Communicating "transfer" flows via unprivileged ethdevs may not
 >> >> +	 * be possible. In order to pick the ethdev suitable for that, the
 >> >
 >> > ethdev -> port
 >>
 >> Why? "Ethdev" is a clearer term for "port".
 >
 >Because the API mention ports, not ethdevs. Example: port_id, not 
ethdev_id.
 >
 >> >> +	 * application should use @p rte_flow_pick_transfer_proxy().
 >> >>   	 */
 >> >>   	uint32_t transfer:1;
 >> > [...]
 >> >> +/**
 >> >
 >> > Please insert an one-line description here.
 >>
 >> Do you want me to make the first line of the description stand out? 
Like
 >> a brief summary / title?
 >
 >Yes like a title.
 >A proposal: "Get the proxy port able to manage transfer rules in 
e-switch."
 >
 >> >> + * 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
 >> >
 >> > Is "e-switch" a common word? Or should we say "embedded switch"?
 >>
 >> Term "e-switch" is a contraction for "embedded switch". I'd go for the
 >> short version.
 >>
 >> >> + * 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").
 >> >
 >> > I recognize the nice right-alignment, but I think this text can be 
shorter.
 >>
 >> No right-alignment here: the last line in the block is not aligned. I
 >> wasn't looking to make this paragraph shorter or longer. I was looking
 >> to introduce the problem clearly. If you believe that I failed to do 
so,
 >> could you please highlight the parts which sound misleading to you.
 >
 >The words misleading me are: "application", "lying beneath", "never",
 >"sitting on top", "entry point".
 >I prefer not talking about application, and avoid story telling style.
 >
 >> >> + *
 >> >> + * This API is a helper to find such "transfer proxy" for a 
given ethdev.
 >
 >I suggest something like this:
 >"
 >The transfer flow must be managed by privileged ports.
 >For some devices, all ports are privileged,
 >while some allow only one port to control the e-switch.
 >This function returns a port (proxy) in the same e-switch domain,
 >usable for managing transfer rules.
 >"
 >
 >> >> + *
 >> >> + * @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.
 >> >> + *
 >> >> + * @param port_id
 >> >> + *   ID of the ethdev in question
 >> >
 >> > The rest of the API says "port", not "ethdev".
 >> > Here I would suggest "ID of the port to look from."
 >>
 >> The argument name is "port_id". This is for consistency with other API
 >> signatures across DPDK. But, in comments, I'd stick with "ethdev". It's
 >> clearer than "port".
 >>
 >> How about "ID of the ethdev to find the proxy for"?
 >
 >I like it except "ethdev" :)

-- 
Ivan M

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [dpdk-dev] [RFC PATCH] ethdev: clarify flow attribute and action port ID semantics
@ 2021-09-07 12:51 Ivan Malov
  2021-10-05  0:36 ` [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev Ivan Malov
  0 siblings, 1 reply; 8+ messages in thread
From: Ivan Malov @ 2021-09-07 12:51 UTC (permalink / raw)
  To: dev; +Cc: Ori Kam, Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko

Problems:

1) Existing item PORT_ID and action PORT_ID are ambiguous because
   one can consider the port in question as either an ethdev or
   an embedded switch entity wired to it, as per the use case,
   which is not expressed clearly in code and documentation.

2) Attributes "ingress" and "egress" may not make sense in flows
   with "transfer" attribute for some PMDs. Furthermore, such
   PMDs may face a related problem (see below).

3) A PMD may not be able to handle "transfer" rules on all ethdevs
   it serves. It may have only one (admin) ethdev capable of that.
   Applications should be able to take this into account and
   submit "transfer" rules on that specific ethdev. However,
   meaning of attributes "ingress" and "egress" might be
   skewed in this case as the ethdev used to make flow
   API calls is just a technical entry point.

In order to solve problem (1)¸ one should recognise the existence
of two major application models: traffic consumer / generator and
a vSwitch / forwarder. To the latter, ethdevs used to perform
Rx / Tx burst calls are simply vSwitch ports. Requesting HW
offloads on these ports implies referring to e-switch ports
that correspond to them at the lowest, e-switch, level.
This way, suggested terminology is clear and concise.

The patch suggests using item / action PORT_ID sub-variants to
disambiguate the meaning. In order to avoid breaking existing
behaviour in Open vSwitch DPDK offload, the sub-variant for
e-switch port is declared with enum value of zero.

In order to solve problems (2) and (3), one needs to recognise
the existence of two paradigms of handling "transfer" rules in
PMDs. If the given PMD needs to "proxy" handling of "transfer"
rules via an admin ethdev, this must not be done implicitly
as the application must know the true ethdev responsible
for handling the flows in order to avoid detaching it
before all "transfer" rules get properly dismantled.

The patch suggests to use a generic helper to let applications
discover the paradigm in use and, if need be, communicate
flow rules through the discovered "proxy" ethdev.

Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
---
This proposal is an alternative to previously suggested RFCs, [1] and [2].
It covers several related problems and suggests clear API contract to
let vSwitch applications use item PORT_ID and action PORT_ID to refer
to e-switch ports thus preserving existing sense.

[1] https://patches.dpdk.org/project/dpdk/patch/20210601111420.5549-1-ivan.malov@oktetlabs.ru/
[2] https://patches.dpdk.org/project/dpdk/patch/20210903074610.313622-1-andrew.rybchenko@oktetlabs.ru/
---
 lib/ethdev/rte_flow.h | 140 ++++++++++++++++++++++++++++++++----------
 1 file changed, 107 insertions(+), 33 deletions(-)

diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
index 70f455d47d..bf2b5e752c 100644
--- a/lib/ethdev/rte_flow.h
+++ b/lib/ethdev/rte_flow.h
@@ -82,22 +82,32 @@ struct rte_flow_attr {
 	uint32_t ingress:1; /**< Rule applies to ingress traffic. */
 	uint32_t egress:1; /**< Rule applies to egress traffic. */
 	/**
-	 * Instead of simply matching the properties of traffic as it would
-	 * appear on a given DPDK port ID, enabling this attribute transfers
-	 * a flow rule to the lowest possible level of any device endpoints
-	 * found in the pattern.
-	 *
-	 * When supported, this effectively enables an application to
-	 * re-route traffic not necessarily intended for it (e.g. coming
-	 * from or addressed to different physical ports, VFs or
-	 * applications) at the device level.
-	 *
-	 * It complements the behavior of some pattern items such as
-	 * RTE_FLOW_ITEM_TYPE_PHY_PORT and is meaningless without them.
-	 *
-	 * When transferring flow rules, ingress and egress attributes keep
-	 * their original meaning, as if processing traffic emitted or
-	 * received by the application.
+	 * This "transfers" the rule from the ethdev level to the embedded
+	 * switch (e-switch) level, where it's possible to match traffic
+	 * not necessarily going to the ethdev where the flow is created
+	 * and redirect it to endpoints otherwise not necessarily
+	 * accessible from rules having no such attribute.
+	 *
+	 * Applications willing to use attribute "transfer" should detect its
+	 * paradigm implemented inside the PMD. The paradigms are as follows:
+	 *
+	 * - The PMD supports handling "transfer" flow rules on any ethdevs
+	 *   it serves. With this paradigm, rte_flow_pick_transfer_proxy()
+	 *   call returns (-ENOTSUP) for all ethdevs backed by the PMD.
+	 *   Attributes "ingress" and "egress" are valid and preserve
+	 *   their original meaning, at application standpoint. Also,
+	 *   these attributes typically set some implicit filtering.
+	 *
+	 * - The PMD only supports handling "transfer" flow rules on some
+	 *   specific ethdev pointed out by rte_flow_pick_transfer_proxy().
+	 *   Typically, it's an admin PF ethdev backing a group of VF
+	 *   representor ethdevs. In this case, attributes "ingress"
+	 *   and "egress" cannot maintain their original meaning as
+	 *   the ethdev used to handle "transfer" flow rules is
+	 *   just a technical entry point and does not mean any
+	 *   implicit filtering. Attribute "egress" is rejected,
+	 *   and "ingress" (redundant) means traffic ingressing
+	 *   the embedded switch from any of its endpoints.
 	 */
 	uint32_t transfer:1;
 	uint32_t reserved:29; /**< Reserved, must be zero. */
@@ -191,8 +201,9 @@ enum rte_flow_item_type {
 	/**
 	 * [META]
 	 *
-	 * Matches traffic originating from (ingress) or going to (egress) a
-	 * given DPDK port ID.
+	 * Matches traffic originating from (ingress) or going to (egress) the
+	 * port which, depending on the item sub-variant, is the given ethdev
+	 * or the opposite end of the "wire" attached to this ethdev.
 	 *
 	 * See struct rte_flow_item_port_id.
 	 */
@@ -679,29 +690,61 @@ static const struct rte_flow_item_phy_port rte_flow_item_phy_port_mask = {
 };
 #endif
 
+/** Port types for use with item PORT_ID and action PORT_ID */
+enum rte_flow_item_port_id_type {
+	/**
+	 * The port in question is an embedded switch entity connected
+	 * to or represented by the given ethdev / vSwitch port.
+	 *
+	 * +--------------------------+---------------------------+
+	 * |  Ethdev / vSwitch Port   |   Embedded Switch Entity  |
+	 * +--------------------------+---------------------------+
+	 * |         PF / VF         <->       Network Port       |
+	 * +------------------------------------------------------+
+	 * |   PF / VF Representor   <->      PF / VF Itself      |
+	 * +------------------------------------------------------+
+	 */
+	RTE_FLOW_PORT_TYPE_ESWITCH = 0,
+
+	/**
+	 * The port in question is an ethdev or, synonymously,
+	 * a DPDK-backed vSwitch port.
+	 */
+	RTE_FLOW_PORT_TYPE_ETHDEV,
+};
+
 /**
  * RTE_FLOW_ITEM_TYPE_PORT_ID
  *
- * Matches traffic originating from (ingress) or going to (egress) a given
- * DPDK port ID.
+ * Matches traffic originating from (ingress) or going to (egress) the
+ * port which, depending on the item sub-variant, is the given ethdev
+ * or the opposite end of the "wire" attached to this ethdev.
  *
- * Normally only supported if the port ID in question is known by the
- * underlying PMD and related to the device the flow rule is created
- * against.
+ * Typically, the ethdev referring to the port in question must be served
+ * by the same PMD as that of the ethdev used to create the flow rule.
+ * Also, the port must normally belong to the same physical board.
  *
- * This must not be confused with @p PHY_PORT which refers to the physical
- * port of a device, whereas @p PORT_ID refers to a struct rte_eth_dev
- * object on the application side (also known as "port representor"
- * depending on the kind of underlying device).
+ * This must not be confused with item @p PHY_PORT
+ * which refers specifically to a physical port.
  */
+RTE_STD_C11
 struct rte_flow_item_port_id {
-	uint32_t id; /**< DPDK port ID. */
+	/** Synonymous defines for ethdev ID property */
+	union {
+		/** Ethdev (vSwitch port) ID */
+		uint32_t ethdev_id;
+		/** Compatibility alias to avoid breaking legacy applications */
+		uint32_t id;
+	};
+
+	/** Port type (item sub-variant) */
+	enum rte_flow_item_port_id_type type;
 };
 
 /** Default mask for RTE_FLOW_ITEM_TYPE_PORT_ID. */
 #ifndef __cplusplus
 static const struct rte_flow_item_port_id rte_flow_item_port_id_mask = {
-	.id = 0xffffffff,
+	.ethdev_id = 0xffffffff,
 };
 #endif
 
@@ -1976,7 +2019,8 @@ enum rte_flow_action_type {
 	RTE_FLOW_ACTION_TYPE_PHY_PORT,
 
 	/**
-	 * Directs matching traffic to a given DPDK port ID.
+	 * Depending on the port type, directs matching traffic either to the
+	 * given ethdev or to the opposite end of the "wire" attached to it.
 	 *
 	 * See struct rte_flow_action_port_id.
 	 */
@@ -2635,14 +2679,26 @@ struct rte_flow_action_phy_port {
 /**
  * RTE_FLOW_ACTION_TYPE_PORT_ID
  *
- * Directs matching traffic to a given DPDK port ID.
+ * Depending on the port type, directs matching traffic either to the
+ * given ethdev or to the opposite end of the "wire" attached to it.
  *
  * @see RTE_FLOW_ITEM_TYPE_PORT_ID
  */
+RTE_STD_C11
 struct rte_flow_action_port_id {
-	uint32_t original:1; /**< Use original DPDK port ID if possible. */
+	uint32_t original:1; /**< Use original ethdev ID if possible. */
 	uint32_t reserved:31; /**< Reserved, must be zero. */
-	uint32_t id; /**< DPDK port ID. */
+
+	/** Synonymous defines for ethdev ID property */
+	union {
+		/** Ethdev (vSwitch port) ID */
+		uint32_t ethdev_id;
+		/** Compatibility alias to avoid breaking legacy applications */
+		uint32_t id;
+	};
+
+	/** Port type (action sub-variant) */
+	enum rte_flow_item_port_id_type type;
 };
 
 /**
@@ -4288,6 +4344,24 @@ rte_flow_tunnel_item_release(uint16_t port_id,
 			     struct rte_flow_item *items,
 			     uint32_t num_of_items,
 			     struct rte_flow_error *error);
+
+/**
+ * Locate the "proxy" ethdev to handle "transfer" flow rules
+ * for the given ethdev. If the API returns (-ENOTSUP), the
+ * caller should assume that no "proxying" is required.
+ *
+ * @param port_id
+ *   Ethdev ID that potentially needs a "proxy"
+ * @param[out] proxy_port_id
+ *   The "proxy" port through which "transfer" rules must be communicated
+ *
+ * @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);
 #ifdef __cplusplus
 }
 #endif
-- 
2.20.1


^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-06 13:23 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06 13:23 [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev Ivan Malov
  -- strict thread matches above, loose matches on Subject: below --
2021-09-07 12:51 [dpdk-dev] [RFC PATCH] ethdev: clarify flow attribute and action port ID semantics Ivan Malov
2021-10-05  0:36 ` [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev Ivan Malov
2021-10-05  9:22   ` Ori Kam
2021-10-05 12:41     ` Ivan Malov
2021-10-05 16:04       ` Ori Kam
2021-10-05 17:56   ` Thomas Monjalon
2021-10-05 18:22     ` Ivan Malov
2021-10-05 19:04       ` Thomas Monjalon

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