[dpdk-dev] [PATCH 3/6] net/ionic: fully implement remove-on-close

[Andrew Boyer]

Hello Ferruh,

> On Dec 15, 2020, at 8:42 AM, Ferruh Yigit <ferruh.yigit at 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 at 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 at 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);