From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AA763A0C41; Wed, 6 Oct 2021 15:23:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 68D3C41143; Wed, 6 Oct 2021 15:23:53 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id B96B541100 for ; Wed, 6 Oct 2021 15:23:52 +0200 (CEST) Received: from [100.65.5.102] (unknown [5.144.122.56]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 45B267F50A; Wed, 6 Oct 2021 16:23:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 45B267F50A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633526632; bh=XrUwgnZlsWoCoX9AjVYQZuTSny/Z3i4GDTnjLMjf+P8=; h=To:From:Subject:Date; b=JLXUzScfExZ7VP9ELtAMbi3o8uZ8UrabqnrdlI2+Kp1e1sGdgxUpkZGPAKQPRVbiu rfC/tlY79X6ldaDofkmvKf/9ZGvGv9X0ECYrlUH8uLml7fnyy+4xWBctNf4wnTl6d+ 9c7C91SVwMy230u0dKxDIqcHzVQmFuLVWZTINYyM= To: Thomas Monjalon , dev@dpdk.org From: Ivan Malov Message-ID: <284e0e75-3787-4cbf-0264-f55885485f5f@oktetlabs.ru> Date: Wed, 6 Oct 2021 16:23:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ethdev: let apps find transfer admin port for a given ethdev X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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