Re: [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev

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