DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
To: Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev
Date: Wed, 6 Oct 2021 16:23:40 +0300	[thread overview]
Message-ID: <284e0e75-3787-4cbf-0264-f55885485f5f@oktetlabs.ru> (raw)

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

             reply	other threads:[~2021-10-06 13:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 13:23 Ivan Malov [this message]
  -- strict thread matches above, loose matches on Subject: below --
2021-09-07 12:51 [dpdk-dev] [RFC PATCH] ethdev: clarify flow attribute and action port ID semantics Ivan Malov
2021-10-05  0:36 ` [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev Ivan Malov
2021-10-05  9:22   ` Ori Kam
2021-10-05 12:41     ` Ivan Malov
2021-10-05 16:04       ` Ori Kam
2021-10-05 17:56   ` Thomas Monjalon
2021-10-05 18:22     ` Ivan Malov
2021-10-05 19:04       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=284e0e75-3787-4cbf-0264-f55885485f5f@oktetlabs.ru \
    --to=ivan.malov@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).