From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 9D452A05D3 for ; Mon, 25 Mar 2019 14:26:34 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 709F53195; Mon, 25 Mar 2019 14:26:33 +0100 (CET) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by dpdk.org (Postfix) with ESMTP id EC11A2C60 for ; Mon, 25 Mar 2019 14:26:31 +0100 (CET) Received: by mail-wm1-f67.google.com with SMTP id t124so9042229wma.4 for ; Mon, 25 Mar 2019 06:26:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=zxGAcNyyichEjzRhHKZzQO2NiiDdExABZvefXlxPeW8=; b=HqOmkX2dWvkO73gAp8hz8s11PTGDfaZh+5qQRHal6Jlt4yaDGM/0ilC5tlXc2bp9IM YT/U+vsu3/oy276npnsGKcWWi/SZZg9EmXjxA7pYBFZhIzZDjSPN2JewnIZ10/gAslcQ bygrBtXXjO8r8xmfwn29Eg5CtwSydUBLXRNDPjUFaD0Ow4+6V2Z0lhcFdia4zlzKImtU rPl4H8/0rs+Vei5YmXjseNVUvwjeF/5lw9IKwnykOFD9Zgo8vR0AbRJAw6Ov29ACD7m0 IjCi3/lKhhI+PkPuBLVKD9MQITkGoylUqZ+G3/5G0Q0eSTXIrAplOF3B+8eYVmR1NKhu NgbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=zxGAcNyyichEjzRhHKZzQO2NiiDdExABZvefXlxPeW8=; b=TTAC7eanOPPVSFGBLCyI0ya4BPXBM8pc+B1UQKtLUJSo6QHaBP5BGya7HKjwnGG9QM oqUfLnKf7fWty4EDIBpJWbV06TsZTNOEHrgS/065AJ+XwiAjnkOGcZhMktpBIDpZyyKC vqrFxMLrcZOcjDEqWTMHM1jWc88Yjn+IHEH9rQYYuaRW8IFlbDUP9oi5hOjX0WZa5ks2 cFDPgn2uCKs7m2LoPpvCjilRKICBuOGtKHEQxo8jf1pUNk35ABORz7x1LiL3H8NwyzqI 4ldHBw7o4N7lvQhj5S8bvaLDFgZ2rWw1BCUwxsq9CnCx2iaqfJH19Y84XQYt3UMUih3T KvyQ== X-Gm-Message-State: APjAAAWkjom9BlJRzIcYo+K0lt6tleMmyxtToqgXTtSX2/rS5TesR8Rf dIdFpVbw49XI5YF94AZXkj4bQw== X-Google-Smtp-Source: APXvYqz4Kwv2wl95rRCtWPNDycgIsAbhG/r2y4hgsPVxqn+NquSsKxwZdiOx+uBIYOCc17mgubjOZw== X-Received: by 2002:a1c:40c3:: with SMTP id n186mr5725419wma.3.1553520391291; Mon, 25 Mar 2019 06:26:31 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id z9sm19440943wmf.12.2019.03.25.06.26.29 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 25 Mar 2019 06:26:29 -0700 (PDT) Date: Mon, 25 Mar 2019 14:26:27 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Ferruh Yigit Cc: Thomas Monjalon , Hyong Youb Kim , "John Daley (johndale)" , Andrew Rybchenko , Qi Zhang , "dev@dpdk.org" , Shahaf Shuler , Jerin Jacob , David Marchand , Maxime Coquelin , Konstantin Ananyev , Hemant Agrawal , Stephen Hemminger Message-ID: <20190325132627.hn64wxypxzzb4hz7@bidouze.vm.6wind.com> References: <20190305055659.3095-1-hyonkim@cisco.com> <3429576.gmczXWLcQl@xps> <09e2672e-c94f-5f7d-84e3-343a7be7452e@intel.com> <3306757.Lg3ZuM36Da@xps> <698fd89f-10c1-41a3-2b60-6a77783716e8@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <698fd89f-10c1-41a3-2b60-6a77783716e8@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2] net/enic: add private API to set ingress VLAN rewrite mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Message-ID: <20190325132627.3_NOormFO2yphHQY1EhJ25Cc2wThkzdwk1I2goHQyGU@z> On Mon, Mar 25, 2019 at 11:49:20AM +0000, Ferruh Yigit wrote: > On 3/20/2019 11:46 AM, Thomas Monjalon wrote: > > 20/03/2019 11:45, Ferruh Yigit: > >> On 3/19/2019 8:30 PM, Thomas Monjalon wrote: > >>> 19/03/2019 19:00, Ferruh Yigit: > >>>> On 3/19/2019 5:36 PM, Thomas Monjalon wrote: > >>>>> 19/03/2019 18:29, Ferruh Yigit: > >>>>>> On 3/14/2019 10:04 PM, Thomas Monjalon wrote: > >>>>>>> 14/03/2019 03:58, Hyong Youb Kim: > >>>>>>>> On Wed, Mar 13, 2019 at 10:29:53PM +0100, Thomas Monjalon wrote: > >>>>>>>>> 13/03/2019 22:11, John Daley (johndale): > >>>>>>>>>> From: Thomas Monjalon > >>>>>>>>>>> 13/03/2019 19:32, Ferruh Yigit: > >>>>>>>>>>>> On 3/5/2019 7:11 AM, Hyong Youb Kim wrote: > >>>>>>>>>>>>> The driver currently has a devarg to set the rewrite mode during > >>>>>>>>>>>>> init. Some apps want to programatically set it after running > >>>>>>>>>>>>> rte_eal_init() and finding that ports are VIC. Add a private > >>>>>>>>>>>>> function to support such applications. > >>>>>>>>>>>> > >>>>>>>>>>>> It is not good idea to have PMD specific APIs (although we already have > >>>>>>>>>>> some). > >>>>>>>>>>>> > >>>>>>>>>>>> Specific to this case, as far as I can see it is to pass a config > >>>>>>>>>>>> value and do the action related to it, what would you think having a > >>>>>>>>>>>> generic key/value set/get API in ethdev for this? Similar to rawdev > >>>>>>>>>>> get_attr/set_attr [1]? > >>>>>>>>>>>> > >>>>>>>>>>>> My concern is it may turn into something like ioctl with many things > >>>>>>>>>>>> pushed to it, and cause possible duplication ... > >>>>>>>>>>> > >>>>>>>>>>> Yes, it is clearly ioctl style. > >>>>>>>>>>> > >>>>>>>>>>> Please could you explain more what is the rewrite mode? > >>>>>>>>>>> Does it apply to the port or the queue? > >>>>>>>>>>> > >>>>>>>>>> It applies to a port. By default the Cisco VIC VLAN tags every packet on ingress even if they were untagged coming in on the wire. They are tagged with VLAN 0 or a VLAN id programmed into the NIC depending on the configuration. Its part of the original design, to maintain priority bits, ancient history. > >>>>>>>>>> > >>>>>>>>>> Some apps don't like this (VPP) or take a slower path (OVS). Hyong added a ig-vlan-rewrite=untag devarg to disable this (leave untagged/default vlan packets untagged) during rte_eal_init and this is helpful for OVS, but VPP likes to set the rewrite mode after rte_eal_init() and finding the ports are VIC ports. So that is the reasoning behind the private API call. > >>>>>>>>> > >>>>>>>>> It looks like an application will always set this flag or never. > >>>>>>>>> So I don't see the need for an API function. > >>>>>>>>> Why VPP prefers set this flag later? > >>>>>>>>> Would it be better to have some driver-specific flags, no matter the ports? > >>>>>>>> > >>>>>>>> As is, there seem to be two ways apps deal with NIC-specific tweaks/quirks. > >>>>>>>> > >>>>>>>> 1. Leave everything to the user. > >>>>>>>> > >>>>>>>> Let the human user specify NIC-specific settings (e.g. devarg, > >>>>>>>> not-so-standard MTU/MRU, offloads with not-so-uniform behavior). The > >>>>>>>> app simply passes these to DPDK and does nothing else. Devargs are > >>>>>>>> passed to rte_eal_init. Other settings are applied during the > >>>>>>>> configure phase after rte_eal_init. > >>>>>>>> > >>>>>>>> For example, OVS seems to go for a smallest common denominator that > >>>>>>>> works with most NICs out of the box. Otherwiese, it kinda falls into > >>>>>>>> this camp. > >>>>>>>> > >>>>>>>> For a problematic NIC that needs user intervention, troubleshooting > >>>>>>>> goes like this :-) > >>>>>>>> - Install app. > >>>>>>>> - Run with settings that worked on a previous machine. > >>>>>>>> - Some features suddenly do not work. > >>>>>>>> - Google search this and that ("why this does not work on this server?"). > >>>>>>>> - Contact vendors. > >>>>>>>> - Find out this NIC has unexpected behavior. > >>>>>>>> - Set devarg, tweak MTU/MRU, ... ("Oh need to set this for .."). > >>>>>>>> - Now the features work. > >>>>>>>> > >>>>>>>> 2. Hide ugly tweaks from the user. > >>>>>>>> > >>>>>>>> VPP falls into this camp. The user specifies BDFs in the config (no > >>>>>>>> devargs). The app calls rte_eal_init(BDFs), iterates through the > >>>>>>>> discovered ports, applies whatever NIC-specific settings necessary > >>>>>>>> during the configure phase (i.e. do this for vendor A NIC, do that for > >>>>>>>> vendor B NIC), and then start the ports. > >>>>>>>> > >>>>>>>> The ingress vlan rewrite mode is devarg now, so is not usable in this > >>>>>>>> model. One way around it is a private API. Driver specific flags > >>>>>>>> during the configure phase would also work fine. Though, enic might be > >>>>>>>> the only user of those flags. > >>>>>>> > >>>>>>> I think DPDK needs some driver configuration. > >>>>>>> Currently the config is done per device with devargs. > >>>>>>> The next devargs format allow this: > >>>>>>> driver=enic,rewrite=on > >>>>>>> and it can be passed to rte_eal_init(). > >>>>>>> > >>>>>>> We did not progress on the implementation of this format in recent months, > >>>>>>> but you are welcome to help! > >>>>>>> Instead of passing devargs in the whitelist/blacklist options, > >>>>>>> we should introduce a new option, like --dev. > >>>>>> > >>>>>> But it will be still devarg in new implementation. > >>>>> > >>>>> With the new syntax, no need to specify a device. > >>>>> We can match a driver or multiple drivers sharing the same property. > >>>>> > >>>>>> I guess for this use case, there is a need to pass this information from an API. > >>>>>> Options can be: > >>>>>> 1- PMD specific API > >>>>>> 2- Extend ethdev dev_ops for each usecase > >>>>>> 3- Have a generic API, as suggested above > >>>>>> 4- Extend configure to accept flags > >>>>>> > >>>>>> I don't see a winner in above list, each has some problems. Any comment on how > >>>>>> to proceed? > >>>>> > >>>>> I don't see a problem with the devargs approach. > >>>> > >>>> Devargs either passed via command line to DPDK application, or parameter to > >>>> hotplug APIs. > >>> > >>> The application can pass whatever it wants to EAL. > >> > >> This means changing current device probe logic completely, right. > >> Instead of probing everything on start, probe nothing and application add > >> devices via eal (hotplug) API calls with providing devargs. > >> I have no issue with this picture, only it doesn't look soon. > > > > No, I mean probe everything at startup automatically as usual. > > Just need to pass an option to the driver > > during its initialization. > > > >>> In the case described above, the application wants to enable > >>> a mode of the driver for all its devices. > >>> That's why I think the right solution is a driver option, > >>> which can be achieved with the new devargs syntax. > >>> > >>>> If someone wants to use regular probe without any command line argument, and > >>>> later configure the device via an API, can devargs be used? > >>> > >>> This is a scenario different of what is asked above. > >>> In the case of a specific configuration of one device, > >>> we have three choices. > >>> These are your suggestions, with my comments: > >>> 1- PMD specific API > >>> 2- Extend ethdev dev_ops for each usecase > >>> (3- Have a generic API) = choice 2 > >>> (4- Extend configure to accept flags) = choice 1 > >>> This is a choice 3: > >>> - no support of exotic features > >> > >> Not sure if this is a real option for a vendor, HWs has exotic features and they > >> want to enable it, I believe SW should not be the blocker for this. > >> > >> Also I definitely agree that exotic features should not break main/common usage, > >> make it hard to use or make it confusing/complex etc. > >> > >> Until we have a better solution I guess we need to continue with private APIs. > > > > I think the driver option would work, > > but it seems I fail to correctly explain it :) > > > > I see it can work if an application always wants this config option to have > *same* value. So it can set this in eal_init() always. > > This requires "driver=xxx,key=value" kind of support in devargs. > > > John, Hyong, > > I guess some level of devargs support is already there, Thomas/Gaetan can > provide more information on latest status of it, can it be possible to get some > support from you to finalize this effort? > > And when it is ready enic can benefit from it for this usecase. > > Thanks, > ferruh Hi Thomas, Ferruh, John, Hyong, driver=xxx,key=value could work, as it is not dependent on the devargs framework, only on the driver implementation. Nothing specific should be needed from EAL PoV (regarding this feature only). What will be missing is the new devargs support in general. Regarding the new devargs: this dev was stalled 2 versions ago at the --dev inclusion step. This parameter was necessary for PMD maintainers to be able to use the new init path with their drivers and transition to rte_eth_devargs_parse() for devargs parsing. --dev was proposed, but its patch was not kept by Thomas during the final crunch. I probably did not shout loud enough about it and let it go, but I think this was a mistake: this feature was low-risk and central in the transition process (it was in parallel to -w/b and --vdev). -- Gaëtan Rivet 6WIND