[RFC] ethdev: complete closing to free all resources
Checks
Commit Message
After closing a port, it cannot be restarted.
So there is no reason to not free all associated resources.
The last step was done with rte_eth_dev_detach() which is deprecated.
Instead of removing the associated rte_device, the driver should check
if no more port (ethdev, cryptodev, etc) is still open for the device.
Then the device resources can be freed by the driver inside the
dev_close() driver callback operation.
The last ethdev freeing (dev_private and final release), which were done
by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
This patch contains only the change in the close function as RFC.
This idea was presented at Dublin during the "hotplug talk".
---
lib/librte_ethdev/rte_ethdev.c | 5 +++++
lib/librte_ethdev/rte_ethdev.h | 5 +++--
2 files changed, 8 insertions(+), 2 deletions(-)
Comments
On 09/08/2018 02:39 AM, Thomas Monjalon wrote:
> After closing a port, it cannot be restarted.
> So there is no reason to not free all associated resources.
>
> The last step was done with rte_eth_dev_detach() which is deprecated.
> Instead of removing the associated rte_device, the driver should check
> if no more port (ethdev, cryptodev, etc) is still open for the device.
> Then the device resources can be freed by the driver inside the
> dev_close() driver callback operation.
>
> The last ethdev freeing (dev_private and final release), which were done
> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
For me, it sounds more logical to kill dev_close and keep detach.
IMHO, dev_close is artificial and hardly useful. detach is a local pair
to attach.
Anyway it requires update of all drivers as I understand.
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> This patch contains only the change in the close function as RFC.
>
> This idea was presented at Dublin during the "hotplug talk".
> ---
> lib/librte_ethdev/rte_ethdev.c | 5 +++++
> lib/librte_ethdev/rte_ethdev.h | 5 +++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 4c3202505..071fcbd23 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1358,6 +1358,7 @@ void
> rte_eth_dev_close(uint16_t port_id)
> {
> struct rte_eth_dev *dev;
> + struct rte_bus *bus;
>
> RTE_ETH_VALID_PORTID_OR_RET(port_id);
> dev = &rte_eth_devices[port_id];
> @@ -1372,6 +1373,10 @@ rte_eth_dev_close(uint16_t port_id)
> dev->data->nb_tx_queues = 0;
> rte_free(dev->data->tx_queues);
> dev->data->tx_queues = NULL;
> +
> + rte_free(dev->data->dev_private);
> +
> + rte_eth_dev_release_port(dev);
> }
>
> int
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 7070e9ab4..37a757a7a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1797,8 +1797,9 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
>
> /**
> * Close a stopped Ethernet device. The device cannot be restarted!
> - * The function frees all resources except for needed by the
> - * closed state. To free these resources, call rte_eth_dev_detach().
> + * The function frees all port resources.
> + * If there is no more port associated with the underlying device,
> + * the driver should free the device resources.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
10/09/2018 10:03, Andrew Rybchenko:
> On 09/08/2018 02:39 AM, Thomas Monjalon wrote:
> > After closing a port, it cannot be restarted.
> > So there is no reason to not free all associated resources.
> >
> > The last step was done with rte_eth_dev_detach() which is deprecated.
> > Instead of removing the associated rte_device, the driver should check
> > if no more port (ethdev, cryptodev, etc) is still open for the device.
> > Then the device resources can be freed by the driver inside the
> > dev_close() driver callback operation.
> >
> > The last ethdev freeing (dev_private and final release), which were done
> > by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
>
> For me, it sounds more logical to kill dev_close and keep detach.
> IMHO, dev_close is artificial and hardly useful. detach is a local pair
> to attach.
I don't get your point.
In order to free a port, we need close + detach.
We can keep only one.
I choose close because:
1) attach/detach are deprecated
2) probe/close is a more obvious pair
3) we need the driver to free the lower level resources
> Anyway it requires update of all drivers as I understand.
Yes if we want to free all resources.
But close operation in drivers can be completed in later steps.
On 09/10/2018 11:42 AM, Thomas Monjalon wrote:
> 10/09/2018 10:03, Andrew Rybchenko:
>> On 09/08/2018 02:39 AM, Thomas Monjalon wrote:
>>> After closing a port, it cannot be restarted.
>>> So there is no reason to not free all associated resources.
>>>
>>> The last step was done with rte_eth_dev_detach() which is deprecated.
>>> Instead of removing the associated rte_device, the driver should check
>>> if no more port (ethdev, cryptodev, etc) is still open for the device.
>>> Then the device resources can be freed by the driver inside the
>>> dev_close() driver callback operation.
>>>
>>> The last ethdev freeing (dev_private and final release), which were done
>>> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
>> For me, it sounds more logical to kill dev_close and keep detach.
>> IMHO, dev_close is artificial and hardly useful. detach is a local pair
>> to attach.
> I don't get your point.
>
> In order to free a port, we need close + detach.
> We can keep only one.
> I choose close because:
> 1) attach/detach are deprecated
> 2) probe/close is a more obvious pair
> 3) we need the driver to free the lower level resources
Yes, I'm sorry I used bad terminology.
We have probe/remove pair for both PCI and vdev drivers and I mean
that remove is a better candidate to be kept (as a pair for probe which
allocates all resources).
10/09/2018 10:54, Andrew Rybchenko:
> On 09/10/2018 11:42 AM, Thomas Monjalon wrote:
> > 10/09/2018 10:03, Andrew Rybchenko:
> >> On 09/08/2018 02:39 AM, Thomas Monjalon wrote:
> >>> After closing a port, it cannot be restarted.
> >>> So there is no reason to not free all associated resources.
> >>>
> >>> The last step was done with rte_eth_dev_detach() which is deprecated.
> >>> Instead of removing the associated rte_device, the driver should check
> >>> if no more port (ethdev, cryptodev, etc) is still open for the device.
> >>> Then the device resources can be freed by the driver inside the
> >>> dev_close() driver callback operation.
> >>>
> >>> The last ethdev freeing (dev_private and final release), which were done
> >>> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
> >> For me, it sounds more logical to kill dev_close and keep detach.
> >> IMHO, dev_close is artificial and hardly useful. detach is a local pair
> >> to attach.
> > I don't get your point.
> >
> > In order to free a port, we need close + detach.
> > We can keep only one.
> > I choose close because:
> > 1) attach/detach are deprecated
> > 2) probe/close is a more obvious pair
> > 3) we need the driver to free the lower level resources
>
> Yes, I'm sorry I used bad terminology.
> We have probe/remove pair for both PCI and vdev drivers and I mean
> that remove is a better candidate to be kept (as a pair for probe which
> allocates all resources).
OK, yes probe/remove is the pair at EAL level.
But if we want to request removal at ethdev level, rte_eth_dev_close
is the function.
Note that there is no function to request creation of an ethdev port.
Adding a new port is done only by the PMD during probing (rte_bus level).
On 09/12/2018 05:57 PM, Thomas Monjalon wrote:
> 10/09/2018 10:54, Andrew Rybchenko:
>> On 09/10/2018 11:42 AM, Thomas Monjalon wrote:
>>> 10/09/2018 10:03, Andrew Rybchenko:
>>>> On 09/08/2018 02:39 AM, Thomas Monjalon wrote:
>>>>> After closing a port, it cannot be restarted.
>>>>> So there is no reason to not free all associated resources.
>>>>>
>>>>> The last step was done with rte_eth_dev_detach() which is deprecated.
>>>>> Instead of removing the associated rte_device, the driver should check
>>>>> if no more port (ethdev, cryptodev, etc) is still open for the device.
>>>>> Then the device resources can be freed by the driver inside the
>>>>> dev_close() driver callback operation.
>>>>>
>>>>> The last ethdev freeing (dev_private and final release), which were done
>>>>> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
>>>> For me, it sounds more logical to kill dev_close and keep detach.
>>>> IMHO, dev_close is artificial and hardly useful. detach is a local pair
>>>> to attach.
>>> I don't get your point.
>>>
>>> In order to free a port, we need close + detach.
>>> We can keep only one.
>>> I choose close because:
>>> 1) attach/detach are deprecated
>>> 2) probe/close is a more obvious pair
>>> 3) we need the driver to free the lower level resources
>> Yes, I'm sorry I used bad terminology.
>> We have probe/remove pair for both PCI and vdev drivers and I mean
>> that remove is a better candidate to be kept (as a pair for probe which
>> allocates all resources).
> OK, yes probe/remove is the pair at EAL level.
> But if we want to request removal at ethdev level, rte_eth_dev_close
> is the function.
>
> Note that there is no function to request creation of an ethdev port.
> Adding a new port is done only by the PMD during probing (rte_bus level).
The overall picture is too vague. May be it is simply incomplete looking
at the patch only. I understand that restructuring is required looking at
rte_eth_dev_destroy() (which is used by i40e and ixgbe drivers only) and
rte_eth_dev_pci_release() used by other PCI drivers.
Right now it looks symmetric at least on drivers level which provides
init callback to probe to do driver-specific job and uninit callback to
remove to free driver-specific resources.
I can't say if the patch is right or wrong direction since final design
is unclear.
On 9/8/2018 12:39 AM, Thomas Monjalon wrote:
> After closing a port, it cannot be restarted.
> So there is no reason to not free all associated resources.
>
> The last step was done with rte_eth_dev_detach() which is deprecated.
> Instead of removing the associated rte_device, the driver should check
> if no more port (ethdev, cryptodev, etc) is still open for the device.
> Then the device resources can be freed by the driver inside the
> dev_close() driver callback operation.
>
> The last ethdev freeing (dev_private and final release), which were done
> by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> This patch contains only the change in the close function as RFC.
>
> This idea was presented at Dublin during the "hotplug talk".
> ---
> lib/librte_ethdev/rte_ethdev.c | 5 +++++
> lib/librte_ethdev/rte_ethdev.h | 5 +++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 4c3202505..071fcbd23 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1358,6 +1358,7 @@ void
> rte_eth_dev_close(uint16_t port_id)
> {
> struct rte_eth_dev *dev;
> + struct rte_bus *bus;
>
> RTE_ETH_VALID_PORTID_OR_RET(port_id);
> dev = &rte_eth_devices[port_id];
> @@ -1372,6 +1373,10 @@ rte_eth_dev_close(uint16_t port_id)
> dev->data->nb_tx_queues = 0;
> rte_free(dev->data->tx_queues);
> dev->data->tx_queues = NULL;
> +
> + rte_free(dev->data->dev_private);
> +
> + rte_eth_dev_release_port(dev);
These already done in:
rte_eth_dev_pci_generic_remove()
rte_eth_dev_pci_release()
Perhaps all content of rte_eth_dev_pci_release(), including above updates,
should move to rte_eth_dev_close() and rte_eth_dev_pci_generic_remove() call
rte_eth_dev_close() directly.
Just thinking aloud,
driver->probe() called when a new device added.
Application startup can be thought as all devices added one by one. [Perhaps
this can be change in the future to add devices only when explicitly stated]
driver->remove() called when device removed.
When application terminated this path not called at all, perhaps we need a way
to remove all devices one by one on exit.
eth_dev_close(), when eth_dev removed in ethdev layer but device is not removed
in eal level,
I think it make sense for eth_dev_close():
- It does more cleanup, free resources and port_id
- but it may need to do more clean up, like call uninit() and do more driver
internal clean up too, and clean up the hw.
- so call stack can be:
driver->remove()
rte_eth_dev_pci_generic_remove()
eth_dev_close()
dev_uninit() [driver unint function ]
Another question, after eth_dev_close() ethdev is not usable and not able to
restart it back. So why we need eth_dev_close() in addition to dev remove, why
not directly call rte_eth_dev_detach()?
> }
>
> int
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 7070e9ab4..37a757a7a 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1797,8 +1797,9 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
>
> /**
> * Close a stopped Ethernet device. The device cannot be restarted!
> - * The function frees all resources except for needed by the
> - * closed state. To free these resources, call rte_eth_dev_detach().
> + * The function frees all port resources.
> + * If there is no more port associated with the underlying device,
> + * the driver should free the device resources.
> *
> * @param port_id
> * The port identifier of the Ethernet device.
>
28/09/2018 14:46, Ferruh Yigit:
> On 9/8/2018 12:39 AM, Thomas Monjalon wrote:
> > After closing a port, it cannot be restarted.
> > So there is no reason to not free all associated resources.
> >
> > The last step was done with rte_eth_dev_detach() which is deprecated.
> > Instead of removing the associated rte_device, the driver should check
> > if no more port (ethdev, cryptodev, etc) is still open for the device.
> > Then the device resources can be freed by the driver inside the
> > dev_close() driver callback operation.
> >
> > The last ethdev freeing (dev_private and final release), which were done
> > by rte_eth_dev_detach(), are now done at the end of rte_eth_dev_close().
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > This patch contains only the change in the close function as RFC.
> >
> > This idea was presented at Dublin during the "hotplug talk".
> > ---
> > @@ -1372,6 +1373,10 @@ rte_eth_dev_close(uint16_t port_id)
> > dev->data->nb_tx_queues = 0;
> > rte_free(dev->data->tx_queues);
> > dev->data->tx_queues = NULL;
> > +
> > + rte_free(dev->data->dev_private);
> > +
> > + rte_eth_dev_release_port(dev);
>
> These already done in:
> rte_eth_dev_pci_generic_remove()
> rte_eth_dev_pci_release()
>
> Perhaps all content of rte_eth_dev_pci_release(), including above updates,
> should move to rte_eth_dev_close() and rte_eth_dev_pci_generic_remove() call
> rte_eth_dev_close() directly.
Yes I think it is the way to go:
- when removing a rte_device, close all associated ports.
and the reverse can be done in addition:
- when closing the last port of a rte_device, remove it.
> Just thinking aloud,
>
> driver->probe() called when a new device added.
> Application startup can be thought as all devices added one by one. [Perhaps
> this can be change in the future to add devices only when explicitly stated]
>
> driver->remove() called when device removed.
> When application terminated this path not called at all, perhaps we need a way
> to remove all devices one by one on exit.
>
> eth_dev_close(), when eth_dev removed in ethdev layer but device is not removed
> in eal level,
>
> I think it make sense for eth_dev_close():
> - It does more cleanup, free resources and port_id
> - but it may need to do more clean up, like call uninit() and do more driver
> internal clean up too, and clean up the hw.
> - so call stack can be:
> driver->remove()
> rte_eth_dev_pci_generic_remove()
> eth_dev_close()
> dev_uninit() [driver unint function ]
>
>
> Another question, after eth_dev_close() ethdev is not usable and not able to
> restart it back. So why we need eth_dev_close() in addition to dev remove, why
> not directly call rte_eth_dev_detach()?
rte_eth_dev_detach is assuming a 1:1 mapping between
EAL rte_device and rte_eth_dev port.
That's why it is deprecated and going to be removed.
@@ -1358,6 +1358,7 @@ void
rte_eth_dev_close(uint16_t port_id)
{
struct rte_eth_dev *dev;
+ struct rte_bus *bus;
RTE_ETH_VALID_PORTID_OR_RET(port_id);
dev = &rte_eth_devices[port_id];
@@ -1372,6 +1373,10 @@ rte_eth_dev_close(uint16_t port_id)
dev->data->nb_tx_queues = 0;
rte_free(dev->data->tx_queues);
dev->data->tx_queues = NULL;
+
+ rte_free(dev->data->dev_private);
+
+ rte_eth_dev_release_port(dev);
}
int
@@ -1797,8 +1797,9 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
/**
* Close a stopped Ethernet device. The device cannot be restarted!
- * The function frees all resources except for needed by the
- * closed state. To free these resources, call rte_eth_dev_detach().
+ * The function frees all port resources.
+ * If there is no more port associated with the underlying device,
+ * the driver should free the device resources.
*
* @param port_id
* The port identifier of the Ethernet device.