DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Boyer <aboyer@pensando.io>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, Alfredo Cardigliano <cardigliano@ntop.org>
Subject: Re: [dpdk-dev] [PATCH 3/6] net/ionic: fully implement remove-on-close
Date: Wed, 16 Dec 2020 14:52:34 -0500	[thread overview]
Message-ID: <475A40F1-ED76-4690-B2FF-0AEE0D2FE681@pensando.io> (raw)
In-Reply-To: <b426f616-58f0-b18b-5951-eed50e665d5d@intel.com>

Hello Ferruh,

> On Dec 15, 2020, at 8:42 AM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> On 12/10/2020 2:22 PM, Andrew Boyer wrote:
>> This is now required behavior for a PMD.
>> Remove the UNMAINTAINED flag.
>> Signed-off-by: Andrew Boyer <aboyer@pensando.io>
>> ---
>>  MAINTAINERS                      |  2 +-
>>  drivers/net/ionic/ionic_ethdev.c | 41 ++++++++++++++++----------------
>>  drivers/net/ionic/ionic_lif.c    | 15 ++++++++++++
>>  drivers/net/ionic/ionic_lif.h    |  1 +
>>  4 files changed, 38 insertions(+), 21 deletions(-)
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7bc0010f2..fc1b09923 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -840,7 +840,7 @@ F: doc/guides/nics/pfe.rst
>>  F: drivers/net/pfe/
>>  F: doc/guides/nics/features/pfe.ini
>>  -Pensando ionic - UNMAINTAINED
>> +Pensando ionic
>>  M: Andrew Boyer <aboyer@pensando.io>
>>  F: drivers/net/ionic/
>>  F: doc/guides/nics/ionic.rst
>> diff --git a/drivers/net/ionic/ionic_ethdev.c b/drivers/net/ionic/ionic_ethdev.c
>> index 5a360ac08..629d7068b 100644
>> --- a/drivers/net/ionic/ionic_ethdev.c
>> +++ b/drivers/net/ionic/ionic_ethdev.c
>> @@ -958,6 +958,8 @@ ionic_dev_stop(struct rte_eth_dev *eth_dev)
>>  	return err;
>>  }
>>  +static void ionic_unconfigure_intr(struct ionic_adapter *adapter);
>> +
>>  /*
>>   * Reset and stop device.
>>   */
>> @@ -965,6 +967,8 @@ static int
>>  ionic_dev_close(struct rte_eth_dev *eth_dev)
>>  {
>>  	struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>> +	struct ionic_adapter *adapter = lif->adapter;
>> +	uint32_t i;
>>  	int err;
>>    	IONIC_PRINT_CALL();
>> @@ -977,12 +981,21 @@ ionic_dev_close(struct rte_eth_dev *eth_dev)
>>  		return -1;
>>  	}
>>  -	err = eth_ionic_dev_uninit(eth_dev);
>> -	if (err) {
>> -		IONIC_PRINT(ERR, "Cannot destroy LIF: %d", err);
>> -		return -1;
>> +	ionic_lif_free_queues(lif);
>> +
>> +	IONIC_PRINT(NOTICE, "Removing device %s", eth_dev->device->name);
>> +	ionic_unconfigure_intr(adapter);
>> +
>> +	for (i = 0; i < adapter->nlifs; i++) {
>> +		lif = adapter->lifs[i];
>> +		rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit);
> 
> Should 'ionic_lif_stop()' & 'ionic_lif_free_queues()' be called per 'lif' here?

You are correct. A future patch removes multi-lif support - it is unused. So this is a result of how I broke up the changes. See below.

>>  	}
>>  +	ionic_port_reset(adapter);
>> +	ionic_reset(adapter);
>> +
>> +	rte_free(adapter);
>> +
> 
> In ionic, an ethdev is created per 'lif' during probe and when one of the ethdev closed, all 'lif' destroyed and 'adapter' freed, so all ethdev should be unusable at this stage.
> 1) First better to document this behavior in the commit log, and as overall can you please prefer more descriptive commit logs.

Sure, I will add more detail to commit logs.

> 2) What happens to the ethdev statuses, 'lif' destroyed and ethdev are not usable but ethdev status not updated or freed?
> What happens if the application tries to access to ethdev of another 'lif' after 'ionic_dev_close()'?

I see what you’re saying, we get here from eth_dev_ops.dev_close -> ionic_dev_close(), so the first ethdev to get closed brings down the adapter etc.

In reality we are going to have one adapter <-> one lif <-> one ethdev, so closing the ethdev will be the end of everything. (Just for this PF/VF; they are independent.)
Rather than doing any design work to fix this I will reorder the patches to take out multi-lif support first.

-Andrew

> 
>>  	return 0;
>>  }
>>  @@ -1280,10 +1293,7 @@ static int
>>  eth_ionic_pci_remove(struct rte_pci_device *pci_dev __rte_unused)
>>  {
>>  	char name[RTE_ETH_NAME_MAX_LEN];
>> -	struct ionic_adapter *adapter = NULL;
>>  	struct rte_eth_dev *eth_dev;
>> -	struct ionic_lif *lif;
>> -	uint32_t i;
>>    	/* Adapter lookup is using (the first) eth_dev name */
>>  	snprintf(name, sizeof(name), "net_%s_lif_0",
>> @@ -1291,19 +1301,10 @@ eth_ionic_pci_remove(struct rte_pci_device *pci_dev __rte_unused)
>>  
> 
> Should remove '__rte_unused'

OK

> 
>>  	eth_dev = rte_eth_dev_allocated(name);
>>  	if (eth_dev) {
>> -		lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>> -		adapter = lif->adapter;
>> -	}
>> -
>> -	if (adapter) {
>> -		ionic_unconfigure_intr(adapter);
>> -
>> -		for (i = 0; i < adapter->nlifs; i++) {
>> -			lif = adapter->lifs[i];
>> -			rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit);
>> -		}
>> -
>> -		rte_free(adapter);
>> +		ionic_dev_close(eth_dev);
>> +	} else {
>> +		IONIC_PRINT(WARNING, "Cannot find device %s",
>> +			pci_dev->device.name);
> 
> Not sure if this warning is correct, if there is no allocated ethdev this is not an error, this means there is nothing to remove and function can continue with 'return 0'

OK, I will reduce it to DEBUG level.

>>  	}
>>    	return 0;
>> diff --git a/drivers/net/ionic/ionic_lif.c b/drivers/net/ionic/ionic_lif.c
>> index 875c7e585..e213597ee 100644
>> --- a/drivers/net/ionic/ionic_lif.c
>> +++ b/drivers/net/ionic/ionic_lif.c
>> @@ -927,6 +927,21 @@ ionic_lif_free(struct ionic_lif *lif)
>>  	}
>>  }
>>  +void
>> +ionic_lif_free_queues(struct ionic_lif *lif)
>> +{
>> +	uint32_t i;
>> +
>> +	for (i = 0; i < lif->ntxqcqs; i++) {
>> +		ionic_dev_tx_queue_release(lif->eth_dev->data->tx_queues[i]);
>> +		lif->eth_dev->data->tx_queues[i] = NULL;
>> +	}
>> +	for (i = 0; i < lif->nrxqcqs; i++) {
>> +		ionic_dev_rx_queue_release(lif->eth_dev->data->rx_queues[i]);
>> +		lif->eth_dev->data->rx_queues[i] = NULL;
>> +	}
>> +}
>> +
>>  int
>>  ionic_lif_rss_config(struct ionic_lif *lif,
>>  		const uint16_t types, const uint8_t *key, const uint32_t *indir)
>> diff --git a/drivers/net/ionic/ionic_lif.h b/drivers/net/ionic/ionic_lif.h
>> index b80931c61..bf010716e 100644
>> --- a/drivers/net/ionic/ionic_lif.h
>> +++ b/drivers/net/ionic/ionic_lif.h
>> @@ -122,6 +122,7 @@ int ionic_lifs_size(struct ionic_adapter *ionic);
>>    int ionic_lif_alloc(struct ionic_lif *lif);
>>  void ionic_lif_free(struct ionic_lif *lif);
>> +void ionic_lif_free_queues(struct ionic_lif *lif);
>>    int ionic_lif_init(struct ionic_lif *lif);
>>  void ionic_lif_deinit(struct ionic_lif *lif);


  reply	other threads:[~2020-12-16 19:52 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-10 14:22 [dpdk-dev] [PATCH 0/6] net/ionic: fixes for stop and start Andrew Boyer
2020-12-10 14:22 ` [dpdk-dev] [PATCH 1/6] net/ionic: preserve RSS state unless RETA size changes Andrew Boyer
2020-12-10 14:22 ` [dpdk-dev] [PATCH 2/6] net/ionic: preserve RX mode across LIF stop/start Andrew Boyer
2020-12-10 14:22 ` [dpdk-dev] [PATCH 3/6] net/ionic: fully implement remove-on-close Andrew Boyer
2020-12-15 13:42   ` Ferruh Yigit
2020-12-16 19:52     ` Andrew Boyer [this message]
2020-12-10 14:22 ` [dpdk-dev] [PATCH 4/6] net/ionic: improve link state handling Andrew Boyer
2020-12-10 14:22 ` [dpdk-dev] [PATCH 5/6] net/ionic: improve queue " Andrew Boyer
2020-12-10 14:22 ` [dpdk-dev] [PATCH 6/6] net/ionic: stop queues when LIF is stopped Andrew Boyer
2020-12-15 14:01 ` [dpdk-dev] [PATCH 0/6] net/ionic: fixes for stop and start Ferruh Yigit
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 0/7] " Andrew Boyer
2021-01-11 17:08   ` Ferruh Yigit
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 1/7] net/ionic: preserve RSS state unless RETA size changes Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 2/7] net/ionic: preserve Rx mode across LIF stop/start Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 3/7] net/ionic: remove multi-LIF support Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 4/7] net/ionic: fully implement remove-on-close Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 5/7] net/ionic: improve link state handling Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 6/7] net/ionic: improve queue " Andrew Boyer
2020-12-16 21:12 ` [dpdk-dev] [PATCH v2 7/7] net/ionic: stop queues when LIF is stopped Andrew Boyer

Reply instructions:

You may reply publicly 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=475A40F1-ED76-4690-B2FF-0AEE0D2FE681@pensando.io \
    --to=aboyer@pensando.io \
    --cc=cardigliano@ntop.org \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).