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 EB47BA0A0A; Thu, 3 Jun 2021 10:43:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D09D840E78; Thu, 3 Jun 2021 10:43:03 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 6946E40DF6 for ; Thu, 3 Jun 2021 10:43:02 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (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 0CE307F4F8; Thu, 3 Jun 2021 11:43:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 0CE307F4F8 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1622709782; bh=kCwr3ZXmtO7PSutS0JCV7158I0WOKZVYHT3Xhsfc8yI=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=B0zDz5jd1ekQjLgv+PzAsQGOLXEsOFwvR30pGcD6HaPSyXn1S1wrVxdac64yqkOFM 3SjtzOZMjIabzn4CmvpIt4tZUFDwissgNkeQCnCxwoWziK5+grFXdPh6ONouYhBkvE KIBkdW5b8N+K5HLqAAaTW0WhkZqKuIFnp7y7dsiw= To: Ori Kam , Ivan Malov , "dev@dpdk.org" Cc: NBU-Contact-Thomas Monjalon , Ferruh Yigit , Ray Kinsella , Neil Horman , Eli Britstein , Ilya Maximets References: <20210527082504.3495-1-ivan.malov@oktetlabs.ru> <6175cb60-5d9a-832a-fa07-32326018661c@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <81191eb5-7a74-2bd3-00e2-0250ef9ee689@oktetlabs.ru> Date: Thu, 3 Jun 2021 11:43:01 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: add support for testpmd-compliant flow rule dumping 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 Ori, Cc Eli and Ilya since I think OvS could be interested in the feature. On 6/3/21 11:25 AM, Ori Kam wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Rybchenko >> >> Hi Ori, >> >> On 6/2/21 4:32 PM, Ori Kam wrote: >>> Hi Ivan, >>> >>>> -----Original Message----- >>>> From: Ivan Malov >>>> >>>> Hi Ori, >>>> >>>> Your review efforts are much appreciated. I understand your concern >>>> about the partial item/action coverage, but there are some points to >>>> be considered when addressing it: >>>> - It's anyway hardly possible to use the printed flow directly in >>>> testpmd if it contains "opaque", or "PMD-specific", items/actions in >>>> terms of the tunnel offload model. These items/actions have to be >>>> omitted when printing the flow, and their absence in the resulting >>>> string means that copy/pasting the flow to testpmd isn't helpful in >>>> this particular case. >>> I fully agree with you that some of the rules can't be printed. That is why. >>> I'm not sure having partial solution is the way to go. >> >> Sorry, I disagree that possibility to cover 99% and impossibility to cover just >> 1% is the reason to discard. >> > > I agree with you that 99% is better than 0 😊 but is this patch 99%? > maybe we can agree even if it is 70% it is good for this patch. Hold on. Here we're talking about a theoretical possibility to cover 99%. Coverage in this patch is discussed below in terms of "the most basic commands". . >>> If OVS for example cares about >>> some of the item/action, maybe this log should be on their part. >> >> OvS does and as far as I can see already has bugs there. >> Of course, nobody says that it is testpmd-complient format, but it definitely >> looks so. >> >> Anyway, it sounds strange do duplicate the functionality in many-many DPDK >> apps. Of course, it removes the headache from DPDK maintainers, but it is >> hardly friendly to DPDK community in general. >> > > Fully agree with you that if some feature is used by number of applications, then it is better > or at least nicer to have it in DPDK, but my question is that, the current patch is for the OVS use case > from my understanding and not considering any other application. So, in this case > do we want it in DPDK? The primary goal, in fact, is our testing harness :) OvS is just an open source example. We could easily add it to our code but decided that it would be useful in DPDK since seen such messages in OvS logs. >>>> - There's action ENCAP which also can't be fully represented by the >>>> tool in question, simply because it has no parameters. In tespmd, one >>>> first has to issue "set vxlan" command to configure the encap. >>>> header, whilst "vxlan" token in the flow rule string just refers to >>>> the previously set encap. parameters. The suggested flow print helper >>>> can't reliably print these two components ("set vxlan" and the flow >>>> rule itself) as they belong to different testpmd command strings. >>>> >>> Again, I agree with you but like my above answer, do we want a partial >>> solution in DPDK? >> >> My answer is YES. >> > > I can live with such decision but like I said above it depends on the use case > and how partial it is. See above. >>>> As you might see, completeness of the solution wouldn't necessarily >>>> be reachable, even if full item/action coverage was provided. >>>> >>>> As for the item/action coverage itself, it's rather controversial. On >>>> the one hand, yes, we should probably try to cover more items and >>>> actions in the suggested patch, to the extent allowed by our current >>>> priorities. But on the other hand, the existing coverage might not be >>>> that poor: it's fairly elaborate and at least allows to print the >>>> most common flow rules. >>>> >>> That is my main issue you are going to push something that is good for >>> you and maybe some other cases, but it can't be used by all >>> application, even with the most basic commands like encap. >> >> Isn't it fair: if one wants to use something, be ready to help and extend it. No >> pain, no gain :) Jokes aside - we're ready to support "the most basic >> commands", just list it, but do not say everything is basic. "everything" will >> delay the feature and simply unfair to demand (IMHO). >> >> IMHO, the feature would make flow API more friendly and easier to debug, >> report bugs etc. >> > I fully agree that if someone wants functionality, he should work for it. > But as a developer of rte_flow and maintainer I need to ask who will add the new > features/missing features? Should we enforce that each developer when coding a new > item/action will add it to the print? > Or just users that care about such log will add it? It is a good and valid questions. First of all we can help with (or just completely take it) to maintain the file. Second basically any option from above is OK for me. My personal preference would be to require implementation for new RTE flow features. In fact, testpmd may start to use it to list created rules etc. We'll try to make it easier to add new items and actions support. > To summarize. > I think the following question must be answer before deciding: > 1. how many apps are going to use this feature? I'll keep OvS maintainers to answer if OvS would like to use it. We'll definitely use it (either from DPDK or from internal code base if it is not accepted), but I guess not open source projects may be not taken into account. > 2. it the coverage sufficient? I hope yes, since it tries to warn about not supported features. I.e. it does not lie simply skipping not supported bits. > 3. who is responsible to update it? (each developer/the interested party member?) See above. I hope to see more answers here. We'll update it when we need more items/actions to dump. Thanks, Andrew. > Best, > Ori > >>>> Yes, macros and some other cunning ways to cover more flow specifics >>>> might come in handy, but, at the same time, can be rather error prone. >>>> Sometimes it's more robust to just write the code out in full. >>>> >>> I'm always in favor of easy of extra complex but too hard is also not good. >>> >>> Thanks, >>> Ori >>>> Thank you. >>>> >>>> On 30/05/2021 10:27, Ori Kam wrote: >>>>> Hi Ivan, >>>>> >>>>> First nice idea and thanks for the picking up the ball. >>>>> >>>>> Before a detail review, >>>>> The main thing I'm concerned about is that this print will be >>>>> partially >>>> supported, >>>>> I know that you covered this issue by printing unknown for >>>>> unsupported >>>> item/actions, >>>>> but this will mean that it is enough that one item/action is not >>>>> supported >>>> and already the >>>>> flow can't be used in testpmd. >>>>> To get full support it means that the developer needs to add such >>>>> print >>>> with each new >>>>> item/action. I agree it is possible, but it has high overhead for each >> feature. >>>>> >>>>> Maybe we should somehow create a macros for the prints or other >>>>> easier >>>> to support ways. >>>>> >>>>> For example, just printing the ipv4 has 7 function calls inside of >>>>> it each one >>>> with error checking, >>>>> and I'm not counting the dedicated functions. >>>>> >>>>> >>>>> >>>>> Best, >>>>> Ori >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Ivan Malov >>>>>> Sent: Thursday, May 27, 2021 11:25 AM >>>>>> To: dev@dpdk.org >>>>>> Cc: NBU-Contact-Thomas Monjalon ; Ferruh >>>> Yigit >>>>>> ; Andrew Rybchenko >>>>>> ; Ori Kam ; Ray >>>>>> Kinsella ; Neil Horman >>>>>> Subject: [RFC PATCH] ethdev: add support for testpmd-compliant flow >>>> rule >>>>>> dumping >>>>>> >>>>>> DPDK applications (for example, OvS) or tests which use RTE flow >>>>>> API >>>> need to >>>>>> log created or rejected flow rules to help to recognise what goes >>>>>> right or wrong. From this standpoint, testpmd-compliant format is >>>>>> nice for the purpose because it allows to copy-paste the flow rules >>>>>> and debug using testpmd. >>>>>> >>>>>> Recognisable pattern items: >>>>>> VOID, VF, PF, PHY_PORT, PORT_ID, ETH, VLAN, IPV4, IPV6, UDP, TCP, >>>> VXLAN, >>>>>> NVGRE, GENEVE, MARK, PPPOES, PPPOED. >>>>>> >>>>>> Recognisable actions: >>>>>> VOID, JUMP, MARK, FLAG, QUEUE, DROP, COUNT, RSS, PF, VF, >>>> PHY_PORT, >>>>>> PORT_ID, OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID, >>>>>> OF_SET_VLAN_PCP, VXLAN_ENCAP, VXLAN_DECAP. >>>>>> >>>>>> Recognisable RSS types (action RSS): >>>>>> IPV4, FRAG_IPV4, NONFRAG_IPV4_TCP, NONFRAG_IPV4_UDP, >>>>>> NONFRAG_IPV4_OTHER, IPV6, FRAG_IPV6, NONFRAG_IPV6_TCP, >>>>>> NONFRAG_IPV6_UDP, NONFRAG_IPV6_OTHER, IPV6_EX, >> IPV6_TCP_EX, >>>>>> IPV6_UDP_EX, L3_SRC_ONLY, L3_DST_ONLY, L4_SRC_ONLY, >>>> L4_DST_ONLY. >>>>>> >>>>>> Unrecognised parts of the flow specification are represented by >>>>>> tokens "{unknown}" and "{unknown bits}". Interested parties are >>>>>> welcome to extend this tool to recognise more items and actions. >>>>>> >>>>>> Signed-off-by: Ivan Malov