DPDK patches and discussions
 help / color / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 5/7] ethdev: do nothing if all-multicast mode is applied again
Date: Tue, 24 Sep 2019 11:54:16 +0300
Message-ID: <11a22a10-ea03-fb02-6c83-eb08a8f4281d@solarflare.com> (raw)
In-Reply-To: <fd3c5310-8ea9-633e-62ea-8edc62f62eb4@intel.com>

On 9/24/19 11:36 AM, Ferruh Yigit wrote:
> On 9/9/2019 1:13 PM, Andrew Rybchenko wrote:
>> Since driver callbacks return status code now, there is no necessity
>> to enable or disable all-multicast mode once again if it is already
>> successfully enabled or disabled.
>>
>> Configuration restore at startup tries to ensure that configured
>> all-multicast mode is applied and start will return error if it fails.
>>
>> Also it avoids theoretical cases when already configured all-multicast
>> mode is applied once again and fails. In this cases it is unclear
>> which value should be reported on get (configured or opposite).
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 8115226c91..e1921e8225 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev,
>>   	}
>>   
>>   	/* replay all multicast configuration */
>> -	if (rte_eth_allmulticast_get(port_id) == 1) {
>> -		ret = rte_eth_allmulticast_enable(port_id);
>> +	/*
>> +	 * use callbacks directly since we don't need port_id check and
>> +	 * would like to bypass the same value set
>> +	 */
>> +	if (rte_eth_allmulticast_get(port_id) == 1 &&
>> +	    *dev->dev_ops->allmulticast_enable != NULL) {
>> +		ret = (*dev->dev_ops->allmulticast_enable)(dev);
> I am for using the API here, it is more abstract instead of adding the dev_ops
> null checks etc. Will there be any downside to use the API?

API does not call operation if value matches and it will exactly match here
for sure since we just get it and applying once again.
Can't say that I like usage callback directly here, but it looks acceptable
for me. I've tried to clarify why it is done this way in the comment above.

> <...>
>
>> @@ -1962,16 +1968,17 @@ int
>>   rte_eth_allmulticast_enable(uint16_t port_id)
>>   {
>>   	struct rte_eth_dev *dev;
>> -	uint8_t old_allmulticast;
>> -	int diag;
>> +	int diag = 0;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP);
>> -	old_allmulticast = dev->data->all_multicast;
>> -	diag = (*dev->dev_ops->allmulticast_enable)(dev);
>> -	dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast;
>> +
>> +	if (dev->data->all_multicast == 0) {
> What about adding this check even before 'allmulticast_enable' check, so if the
> multicast is already enabled why bother having dev_ops or not:
>
> if (dev->data->all_multicast == 1)
> 	return eth_err(port_id, diag);

Yes, it is a good idea. If so, similar fix up will be required for 
promiscuous mode.

>> +		diag = (*dev->dev_ops->allmulticast_enable)(dev);
>> +		dev->data->all_multicast = (diag == 0) ? 1 : 0;
>> +	}
>>   
>>   	return eth_err(port_id, diag);
>>   }
>> @@ -1980,18 +1987,19 @@ int
>>   rte_eth_allmulticast_disable(uint16_t port_id)
>>   {
>>   	struct rte_eth_dev *dev;
>> -	uint8_t old_allmulticast;
>> -	int diag;
>> +	int diag = 0;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP);
>> -	old_allmulticast = dev->data->all_multicast;
>> -	dev->data->all_multicast = 0;
>> -	diag = (*dev->dev_ops->allmulticast_disable)(dev);
>> -	if (diag != 0)
>> -		dev->data->all_multicast = old_allmulticast;
>> +
>> +	if (dev->data->all_multicast == 1) {
> Same comment with above, can we move this check above..

Yes, will fix in the next version as well. Thanks for review.

>> +		dev->data->all_multicast = 0;
>> +		diag = (*dev->dev_ops->allmulticast_disable)(dev);
>> +		if (diag != 0)
>> +			dev->data->all_multicast = 1;
>> +	}
>>   
>>   	return eth_err(port_id, diag);
>>   }
>>


  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-09 12:13 [dpdk-dev] [PATCH 0/7] ethdev: change allmulticast controls to return status Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 1/7] ethdev: change allmulticast mode controllers to return errors Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 2/7] net/failsafe: check code of allmulticast mode switch Andrew Rybchenko
2019-09-09 12:56   ` Gaëtan Rivet
2019-09-13 21:04     ` Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 3/7] net/bonding: " Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 4/7] ethdev: change allmulticast callbacks to return status Andrew Rybchenko
2019-09-16  7:03   ` Hyong Youb Kim (hyonkim)
2019-09-16  7:29     ` Andrew Rybchenko
2019-09-24  8:27   ` Ferruh Yigit
2019-09-24 12:14     ` Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 5/7] ethdev: do nothing if all-multicast mode is applied again Andrew Rybchenko
2019-09-24  8:36   ` Ferruh Yigit
2019-09-24  8:54     ` Andrew Rybchenko [this message]
2019-09-24 11:03       ` Ferruh Yigit
2019-09-24 11:50         ` Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 6/7] app/testpmd: check code of allmulticast mode switch Andrew Rybchenko
2019-09-09 12:13 ` [dpdk-dev] [PATCH 7/7] examples/ipv4_multicast: check allmulticast enable status Andrew Rybchenko

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=11a22a10-ea03-fb02-6c83-eb08a8f4281d@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox