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

[Thomas Monjalon <thomas@monjalon.net>]

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" :)