Re: [dpdk-dev] [RFC] net/null: add empty promiscuous mode functions

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

On 10/17/2019 4:33 PM, Ferruh Yigit wrote:
> On 10/17/2019 2:43 PM, Bruce Richardson wrote:
>> On Thu, Oct 17, 2019 at 12:05:56PM +0100, Ferruh Yigit wrote:
>>> On 10/17/2019 11:51 AM, Andrew Rybchenko wrote:
>>>> On 10/17/19 1:47 PM, Ferruh Yigit wrote:
>>>>> On 10/17/2019 11:37 AM, Andrew Rybchenko wrote:
>>>>>> On 10/16/19 9:07 PM, Ferruh Yigit wrote:
>>>>>>> On 10/16/2019 4:46 PM, Ciara Power wrote:
>>>>>>>> Adding promiscuous functions prevents sample applications failing when run
>>>>>>>> on this virtual PMD. The sample applications call promiscuous functions,
>>>>>>>> and fail if this function call returns an error, which occurs when the
>>>>>>>> virtual PMD does not support the promiscuous function being called.
>>>>>>>>
>>>>>>>> This change will be implemented for all virtual PMDs that currently do not
>>>>>>>> have existing promiscuous functions. Multicast functions will also be
>>>>>>>> added for virtual PMDs to prevent sample application breakages here also.
>>>>>>> +Andrew
>>>>>>>
>>>>>>> With the some ethdev APIs returning error code, some sample applications stop
>>>>>>> working with virtual interfaces,
>>>>>>>
>>>>>>> We can,
>>>>>>> 1- update sample applications to ignore the errors
>>>>>>> 2- Add dummy dev_ops support to (almost all) virtual PMDs (what this RFC suggests)
>>>>>>>
>>>>>>> (1) puts us back to before the ethdev APIs updated status, and this may be wrong
>>>>>>> for the physical devices case, so I am for this RFC.
>>>>>>>
>>>>>>> Only perhaps we can have some common empty function and keep assigning that one
>>>>>>> to reduce the dummy code, what do you think?
>>>>>> I don't like the idea to have common empty/dummy functions.
>>>>>> If virtual PMD behaves in accordance with enabled promiscuous mode,
>>>>>> it should initialize it properly on init:
>>>>>>        eth_dev->data->promiscuous = 1;
>>>>>> If so, if application requires promiscuous mode, attempt to enable will
>>>>>> do nothing. If application requires non-promiscuous mode, disable will
>>>>>> fail and it is good.
>>>>> It is technically correct that we can't disable promiscuous mode in virtual PMDs
>>>>> but I think mainly we don't really care so it returning error may make the
>>>>> applications fail/exit unnecessarily with virtual PMDs.
>>>>
>>>> If I test virtual PMD promiscuous mode, I would prefer enable/disable
>>>> callback to say me truth.
>>>>
>>>> If application really does not care, it should be in the application code.
>>>
>>> Application can't change this because they may be caring return result for the
>>> physical devices.
>>>
>>> Up until this release these missing dev_ops in virtual PMDs were silently
>>> ignored, now APIs are more strict on this (which is good) but to get close the
>>> previous behavior for virtual PMDs we need to relax on these features (like
>>> saying success on promiscuous disable although it didn't).
>>>
>> The other variable here is how often an app is going to request promiscuous
>> disabling? Given that most ports generally come up in that state anyway,
>> and one needs to request enabling it, surely the disable case is relatively
>> rare? In that case I'd tend to agree with having disabling it returning
>> error for vpmds.
>>
> 
> Yes disabling most probably rare, but still it will generate an error and
> application is failing because of ring PMD promiscuous disable doesn't look
> right to me.
> 
> Perhaps application should differentiate between -ENOTSUP error and operation
> fail error, but that looks to me adding unnecessary complexity to the app.
> 
> With a common function shared by all PMDs for both promisc and allmuticast will
> add a little code and an easier solution.
> 

btw, initialize promiscuous as enabled at PMD init won't help with current APIs
because in API dev_ops check is earlier and will still cause -ENOTSUP.


rte_eth_promiscuous_enable
  RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
  RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->promiscuous_enable, -ENOTSUP);
  if (dev->data->promiscuous == 0)
     diag = (*dev->dev_ops->promiscuous_enable)(dev);
     dev->data->promiscuous = (diag == 0) ? 1 : 0;
  return