Re: [dpdk-dev] [PATCH v2] net/enic: add private API to set ingress VLAN rewrite mode

[Ferruh Yigit <ferruh.yigit@intel.com>]

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 <thomas@monjalon.net>
>>>>>>> 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.
If someone wants to use regular probe without any command line argument, and
later configure the device via an API, can devargs be used?