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