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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/18/19 11:18 AM, Ferruh Yigit wrote:
> 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.

My bad, I think it should be fixed.

> 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