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

[Ferruh Yigit]

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 <thomas at 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.
>>>
>>> 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