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 A1A4BA0547; Thu, 28 Oct 2021 18:24:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8478940E25; Thu, 28 Oct 2021 18:24:26 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3E70F40DDB for ; Thu, 28 Oct 2021 18:24:25 +0200 (CEST) Received: from [100.65.7.157] (unknown [5.144.123.112]) (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 A63177F52E; Thu, 28 Oct 2021 19:24:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru A63177F52E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1635438264; bh=R7mjULqRBv5+g+I8YjL/eSsVThv0/zr0C2r9c4qJtgs=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=gsOTnbXK6/6lKoFrdHqDT421bjZZ88P0ae8UPBMmao7Ax21h3er7qUPg85yshwLnN 25RaF/esyvym7kyagUtvoGxzjFnrNCgHhBBmrSzCxnC/KgRVrblOEFQYeMYcURVEdZ cUIuQp2KP5D+fvQR6YXgwQ/oanwS23MKetN+xX7g= To: Thomas Monjalon Cc: dev@dpdk.org, David Marchand , Ferruh Yigit , Ori Kam , Andrew Rybchenko References: <20211027090003.14556-1-ivan.malov@oktetlabs.ru> <2080007.0klm6OBeOA@thomas> <5927197.slJBfLvSNU@thomas> From: Ivan Malov Message-ID: <43fd9b2e-5c2e-d464-5b24-88a2d8da0e1e@oktetlabs.ru> Date: Thu, 28 Oct 2021 19:24:20 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <5927197.slJBfLvSNU@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ethdev: fine tune error reporting in pick transfer proxy API 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" Good day to you, Thomas. On 27/10/2021 13:57, Thomas Monjalon wrote: > 27/10/2021 11:55, Ivan Malov: >> Hi Thomas, >> >> On 27/10/2021 12:46, Thomas Monjalon wrote: >>> 27/10/2021 11:00, Ivan Malov: >>>> There are PMDs which do not support flow offloads at all. >>>> In such cases, the API in question returns ENOTSUP. This >>>> is too loud. Restructure the code to avoid spamming logs. >>>> >>>> Fixes: 1179f05cc9a0 ("ethdev: query proxy port to manage transfer flows") >>>> >>>> Signed-off-by: Ivan Malov >>>> --- >>>> --- a/lib/ethdev/rte_flow.c >>>> +++ b/lib/ethdev/rte_flow.c >>>> @@ -1335,10 +1335,7 @@ rte_flow_pick_transfer_proxy(uint16_t port_id, uint16_t *proxy_port_id, >>>> const struct rte_flow_ops *ops = rte_flow_ops_get(port_id, error); >>>> struct rte_eth_dev *dev; >>>> >>>> - if (unlikely(ops == NULL)) >>>> - return -rte_errno; >>>> - >>>> - if (ops->pick_transfer_proxy == NULL) { >>>> + if (ops == NULL || ops->pick_transfer_proxy == NULL) { >>>> *proxy_port_id = port_id; >>>> return 0; >>>> } >>> >>> I prefer this logic. >> >> Thank you. >> >>> You could add a comment to say that the current port is the default. >> >> As far as I remember, the comment ("note") is already in place (rte_flow.h). > > I meant adding a comment in the implementation above. Technically, I don't object adding it. But isn't the idea expressed clear enough by the code itself? > >>> There is also this logic in testpmd: >>> >>> port->flow_transfer_proxy = port_id; >>> if (!is_proc_primary()) >>> return; >>> >>> Could we manage secondary process case inside the API? >> >> Shouldn't we manage secondary process in *all* flow APIs then? > > Hmm, yes logically we should not care about secondary process at all in rte_flow. > OK to leave it as is. Thank you. > >>> One more comment, for testpmd, >>> we are calling rte_flow_pick_transfer_proxy even if we do not config any transfer flow. >>> It is called always in init_config_port_offloads(). >>> It looks wrong. Can we call it only when needed? >> >> In which way does it look wrong? Does it inflict error(s), malfunction, >> performance drops? Please elaborate. > > It is testing a function that we don't intend to test in a basic use case. Not really. The original idea is to invoke this API only once, on port (re-)plug and remember the proxy port ID to be used on each flow create invocation. Theoretically, when the new asynchronous flow API arrives, this approach will be even more to the point. > A driver can introduce a malfunction with this API while > we don't use rte_flow at all in the test scenario. Fat chance. Even if that happens, it will draw attention. It is the duty of test-pmd to detect such malfunction after all. If the current code comes across a bug in some driver, it should be good, shouldn't it? Test coverage gets extended, right? -- Ivan M