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

[Andrew Rybchenko]

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.

>>>> Signed-off-by: Ciara Power <ciara.power at intel.com>
>>>> ---
>>>>    drivers/net/null/rte_eth_null.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/net/null/rte_eth_null.c b/drivers/net/null/rte_eth_null.c
>>>> index e2ff41a22..b8472a0cf 100644
>>>> --- a/drivers/net/null/rte_eth_null.c
>>>> +++ b/drivers/net/null/rte_eth_null.c
>>>> @@ -441,11 +441,25 @@ eth_mac_address_set(__rte_unused struct rte_eth_dev *dev,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static int
>>>> +eth_dev_promiscuous_enable(__rte_unused struct rte_eth_dev *dev)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> +eth_dev_promiscuous_disable(__rte_unused struct rte_eth_dev *dev)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static const struct eth_dev_ops ops = {
>>>>    	.dev_start = eth_dev_start,
>>>>    	.dev_stop = eth_dev_stop,
>>>>    	.dev_configure = eth_dev_configure,
>>>>    	.dev_infos_get = eth_dev_info,
>>>> +	.promiscuous_enable = eth_dev_promiscuous_enable,
>>>> +	.promiscuous_disable = eth_dev_promiscuous_disable,
>>>>    	.rx_queue_setup = eth_rx_queue_setup,
>>>>    	.tx_queue_setup = eth_tx_queue_setup,
>>>>    	.rx_queue_release = eth_queue_release,
>>>>