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.ma...@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
