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".
--- 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".
+ * 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?
+ * 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.
+ *
+ * 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.
+ *
+ * @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"?
+ * @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);
--
Ivan M
