[v1,1/2] net/af_xdp: remove resources when port is closed
Checks
Commit Message
Since 18.11, it is suggested that driver should release all its private
resources at the dev_close routine. So all resources previously released
in remove routine are now released at the dev_close routine, and the
dev_close routine will be called in driver remove routine in order to
support removing a device without closing its ports.
Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
flag during probe stage.
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
drivers/net/af_xdp/rte_eth_af_xdp.c | 41 +++++++++++++----------------
1 file changed, 19 insertions(+), 22 deletions(-)
Comments
On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> Since 18.11, it is suggested that driver should release all its private
> resources at the dev_close routine. So all resources previously released
> in remove routine are now released at the dev_close routine, and the
> dev_close routine will be called in driver remove routine in order to
> support removing a device without closing its ports.
>
> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> flag during probe stage.
>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
<...>
> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> if (eth_dev == NULL)
> return -1;
>
> - internals = eth_dev->data->dev_private;
> -
> - rte_ring_free(internals->umem->buf_ring);
> - rte_memzone_free(internals->umem->mz);
> - rte_free(internals->umem);
> -
> - rte_eth_dev_release_port(eth_dev);
I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
'rte_eth_dev_close()' but still needed in '.remove()' path.
> -
> + eth_dev_close(eth_dev);
>
> return 0;
> }
>
29/04/2019 19:00, Ferruh Yigit:
> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> > Since 18.11, it is suggested that driver should release all its private
> > resources at the dev_close routine. So all resources previously released
> > in remove routine are now released at the dev_close routine, and the
> > dev_close routine will be called in driver remove routine in order to
> > support removing a device without closing its ports.
> >
> > Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> > flag during probe stage.
> >
> > Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
> <...>
>
> > @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> > if (eth_dev == NULL)
> > return -1;
> >
> > - internals = eth_dev->data->dev_private;
> > -
> > - rte_ring_free(internals->umem->buf_ring);
> > - rte_memzone_free(internals->umem->mz);
> > - rte_free(internals->umem);
> > -
> > - rte_eth_dev_release_port(eth_dev);
>
> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> 'rte_eth_dev_close()' but still needed in '.remove()' path.
I don't understand your comment.
Calling the close function looks the right thing to do in "remove".
> > -
> > + eth_dev_close(eth_dev);
On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> 29/04/2019 19:00, Ferruh Yigit:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>
> I don't understand your comment.
> Calling the close function looks the right thing to do in "remove".
No concern on calling the 'close'.
My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
>>> -
>>> + eth_dev_close(eth_dev);
>
>
>
30/04/2019 00:28, Ferruh Yigit:
> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> > 29/04/2019 19:00, Ferruh Yigit:
> >> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>> Since 18.11, it is suggested that driver should release all its private
> >>> resources at the dev_close routine. So all resources previously released
> >>> in remove routine are now released at the dev_close routine, and the
> >>> dev_close routine will be called in driver remove routine in order to
> >>> support removing a device without closing its ports.
> >>>
> >>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>> flag during probe stage.
> >>>
> >>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>
> >> <...>
> >>
> >>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>> if (eth_dev == NULL)
> >>> return -1;
> >>>
> >>> - internals = eth_dev->data->dev_private;
> >>> -
> >>> - rte_ring_free(internals->umem->buf_ring);
> >>> - rte_memzone_free(internals->umem->mz);
> >>> - rte_free(internals->umem);
> >>> -
> >>> - rte_eth_dev_release_port(eth_dev);
> >>
> >> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >
> > I don't understand your comment.
> > Calling the close function looks the right thing to do in "remove".
>
> No concern on calling the 'close'.
> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
> >>> -
> >>> + eth_dev_close(eth_dev);
Hi, Ferruh
On 04/29, Ferruh Yigit wrote:
>On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>> Since 18.11, it is suggested that driver should release all its private
>> resources at the dev_close routine. So all resources previously released
>> in remove routine are now released at the dev_close routine, and the
>> dev_close routine will be called in driver remove routine in order to
>> support removing a device without closing its ports.
>>
>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>> flag during probe stage.
>>
>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>
><...>
>
>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>> if (eth_dev == NULL)
>> return -1;
>>
>> - internals = eth_dev->data->dev_private;
>> -
>> - rte_ring_free(internals->umem->buf_ring);
>> - rte_memzone_free(internals->umem->mz);
>> - rte_free(internals->umem);
>> -
>> - rte_eth_dev_release_port(eth_dev);
>
>I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>'rte_eth_dev_close()' but still needed in '.remove()' path.
>
remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
Thanks,
Xiaolong
>> -
>> + eth_dev_close(eth_dev);
>>
>> return 0;
>> }
>>
>
On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> 30/04/2019 00:28, Ferruh Yigit:
>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>> 29/04/2019 19:00, Ferruh Yigit:
>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>> resources at the dev_close routine. So all resources previously released
>>>>> in remove routine are now released at the dev_close routine, and the
>>>>> dev_close routine will be called in driver remove routine in order to
>>>>> support removing a device without closing its ports.
>>>>>
>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>> flag during probe stage.
>>>>>
>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>> if (eth_dev == NULL)
>>>>> return -1;
>>>>>
>>>>> - internals = eth_dev->data->dev_private;
>>>>> -
>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>> - rte_memzone_free(internals->umem->mz);
>>>>> - rte_free(internals->umem);
>>>>> -
>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>
>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>> I don't understand your comment.
>>> Calling the close function looks the right thing to do in "remove".
>>
>> No concern on calling the 'close'.
>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>
> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
private resources.
I assume it is confused with 'rte_eth_dev_close()' ...
>
>>>>> -
>>>>> + eth_dev_close(eth_dev);
>
>
>
On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
> Hi, Ferruh
>
> On 04/29, Ferruh Yigit wrote:
>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>> Since 18.11, it is suggested that driver should release all its private
>>> resources at the dev_close routine. So all resources previously released
>>> in remove routine are now released at the dev_close routine, and the
>>> dev_close routine will be called in driver remove routine in order to
>>> support removing a device without closing its ports.
>>>
>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>> flag during probe stage.
>>>
>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>
>> <...>
>>
>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>> if (eth_dev == NULL)
>>> return -1;
>>>
>>> - internals = eth_dev->data->dev_private;
>>> -
>>> - rte_ring_free(internals->umem->buf_ring);
>>> - rte_memzone_free(internals->umem->mz);
>>> - rte_free(internals->umem);
>>> -
>>> - rte_eth_dev_release_port(eth_dev);
>>
>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>
>
> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
shouldn't really, am I missing something?
>
> Thanks,
> Xiaolong
>
>>> -
>>> + eth_dev_close(eth_dev);
>>>
>>> return 0;
>>> }
>>>
>>
30/04/2019 09:33, Ferruh Yigit:
> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
> > 30/04/2019 00:28, Ferruh Yigit:
> >> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
> >>> 29/04/2019 19:00, Ferruh Yigit:
> >>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
> >>>>> Since 18.11, it is suggested that driver should release all its private
> >>>>> resources at the dev_close routine. So all resources previously released
> >>>>> in remove routine are now released at the dev_close routine, and the
> >>>>> dev_close routine will be called in driver remove routine in order to
> >>>>> support removing a device without closing its ports.
> >>>>>
> >>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
> >>>>> flag during probe stage.
> >>>>>
> >>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> >>>>
> >>>> <...>
> >>>>
> >>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
> >>>>> if (eth_dev == NULL)
> >>>>> return -1;
> >>>>>
> >>>>> - internals = eth_dev->data->dev_private;
> >>>>> -
> >>>>> - rte_ring_free(internals->umem->buf_ring);
> >>>>> - rte_memzone_free(internals->umem->mz);
> >>>>> - rte_free(internals->umem);
> >>>>> -
> >>>>> - rte_eth_dev_release_port(eth_dev);
> >>>>
> >>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
> >>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
> >>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
> >>>
> >>> I don't understand your comment.
> >>> Calling the close function looks the right thing to do in "remove".
> >>
> >> No concern on calling the 'close'.
> >> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
> >
> > rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>
> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
> private resources.
> I assume it is confused with 'rte_eth_dev_close()' ...
>
> >>>>> -
> >>>>> + eth_dev_close(eth_dev);
Ah yes, I overlooked it.
Why not calling rte_eth_dev_close()?
On 4/30/2019 8:55 AM, Thomas Monjalon wrote:
> 30/04/2019 09:33, Ferruh Yigit:
>> On 4/29/2019 11:34 PM, Thomas Monjalon wrote:
>>> 30/04/2019 00:28, Ferruh Yigit:
>>>> On 4/29/2019 9:14 PM, Thomas Monjalon wrote:
>>>>> 29/04/2019 19:00, Ferruh Yigit:
>>>>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>>>>> Since 18.11, it is suggested that driver should release all its private
>>>>>>> resources at the dev_close routine. So all resources previously released
>>>>>>> in remove routine are now released at the dev_close routine, and the
>>>>>>> dev_close routine will be called in driver remove routine in order to
>>>>>>> support removing a device without closing its ports.
>>>>>>>
>>>>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>>>>> flag during probe stage.
>>>>>>>
>>>>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>>>>
>>>>>> <...>
>>>>>>
>>>>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>>>>> if (eth_dev == NULL)
>>>>>>> return -1;
>>>>>>>
>>>>>>> - internals = eth_dev->data->dev_private;
>>>>>>> -
>>>>>>> - rte_ring_free(internals->umem->buf_ring);
>>>>>>> - rte_memzone_free(internals->umem->mz);
>>>>>>> - rte_free(internals->umem);
>>>>>>> -
>>>>>>> - rte_eth_dev_release_port(eth_dev);
>>>>>>
>>>>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>>>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>>>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>>>
>>>>> I don't understand your comment.
>>>>> Calling the close function looks the right thing to do in "remove".
>>>>
>>>> No concern on calling the 'close'.
>>>> My comment was to keep 'rte_eth_dev_release_port()' which this patch removes.
>>>
>>> rte_eth_dev_release_port() is called in eth_dev_close(), isn't it?
>>
>> No, 'eth_dev_close()' is local 'dev_close()' ops, the one to clear driver
>> private resources.
>> I assume it is confused with 'rte_eth_dev_close()' ...
>>
>>>>>>> -
>>>>>>> + eth_dev_close(eth_dev);
>
> Ah yes, I overlooked it.
> Why not calling rte_eth_dev_close()?
>
It may work, but I am for keeping 'dev_close()' and 'rte_eth_dev_release_port()'
as two steps in '.remove()', I think there is no need to include
'rte_eth_dev_close()' API here and its possible/future side affects etc...
On 04/30, Ferruh Yigit wrote:
>On 4/30/2019 3:06 AM, Ye Xiaolong wrote:
>> Hi, Ferruh
>>
>> On 04/29, Ferruh Yigit wrote:
>>> On 4/26/2019 6:09 AM, Xiaolong Ye wrote:
>>>> Since 18.11, it is suggested that driver should release all its private
>>>> resources at the dev_close routine. So all resources previously released
>>>> in remove routine are now released at the dev_close routine, and the
>>>> dev_close routine will be called in driver remove routine in order to
>>>> support removing a device without closing its ports.
>>>>
>>>> Above behavior changes are supported by setting RTE_ETH_DEV_CLOSE_REMOVE
>>>> flag during probe stage.
>>>>
>>>> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
>>>
>>> <...>
>>>
>>>> @@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
>>>> if (eth_dev == NULL)
>>>> return -1;
>>>>
>>>> - internals = eth_dev->data->dev_private;
>>>> -
>>>> - rte_ring_free(internals->umem->buf_ring);
>>>> - rte_memzone_free(internals->umem->mz);
>>>> - rte_free(internals->umem);
>>>> -
>>>> - rte_eth_dev_release_port(eth_dev);
>>>
>>> I thinks we should keep 'rte_eth_dev_release_port()' in '.remove()' path,
>>> the 'RTE_ETH_DEV_CLOSE_REMOVE' flag will take care of this in
>>> 'rte_eth_dev_close()' but still needed in '.remove()' path.
>>>
>>
>> remove() would call eth_dev_close which includes the rte_eth_dev_release_port().
>
>'eth_dev_close()' doesn't call the 'rte_eth_dev_release_port()', and it
>shouldn't really, am I missing something?
Sorry, it's the rte_eth_dev_close that calls rte_eth_dev_release_port, here in
.remove we do need to call rte_eth_dev_release_port explicitly.
will send a new version.
Thanks,
Xiaolong
>
>>
>> Thanks,
>> Xiaolong
>>
>>>> -
>>>> + eth_dev_close(eth_dev);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>
>
@@ -426,6 +426,19 @@ remove_xdp_program(struct pmd_internals *internals)
XDP_FLAGS_UPDATE_IF_NOEXIST);
}
+static void
+xdp_umem_destroy(struct xsk_umem_info *umem)
+{
+ rte_memzone_free(umem->mz);
+ umem->mz = NULL;
+
+ rte_ring_free(umem->buf_ring);
+ umem->buf_ring = NULL;
+
+ rte_free(umem);
+ umem = NULL;
+}
+
static void
eth_dev_close(struct rte_eth_dev *dev)
{
@@ -444,6 +457,9 @@ eth_dev_close(struct rte_eth_dev *dev)
}
(void)xsk_umem__delete(internals->umem->umem);
+
+ xdp_umem_destroy(internals->umem);
+
remove_xdp_program(internals);
}
@@ -459,19 +475,6 @@ eth_link_update(struct rte_eth_dev *dev __rte_unused,
return 0;
}
-static void
-xdp_umem_destroy(struct xsk_umem_info *umem)
-{
- rte_memzone_free(umem->mz);
- umem->mz = NULL;
-
- rte_ring_free(umem->buf_ring);
- umem->buf_ring = NULL;
-
- rte_free(umem);
- umem = NULL;
-}
-
static struct
xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals)
{
@@ -856,6 +859,8 @@ init_internals(struct rte_vdev_device *dev,
eth_dev->dev_ops = &ops;
eth_dev->rx_pkt_burst = eth_af_xdp_rx;
eth_dev->tx_pkt_burst = eth_af_xdp_tx;
+ /* Let rte_eth_dev_close() release the port resources. */
+ eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
return eth_dev;
@@ -923,7 +928,6 @@ static int
rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
{
struct rte_eth_dev *eth_dev = NULL;
- struct pmd_internals *internals;
AF_XDP_LOG(INFO, "Removing AF_XDP ethdev on numa socket %u\n",
rte_socket_id());
@@ -936,14 +940,7 @@ rte_pmd_af_xdp_remove(struct rte_vdev_device *dev)
if (eth_dev == NULL)
return -1;
- internals = eth_dev->data->dev_private;
-
- rte_ring_free(internals->umem->buf_ring);
- rte_memzone_free(internals->umem->mz);
- rte_free(internals->umem);
-
- rte_eth_dev_release_port(eth_dev);
-
+ eth_dev_close(eth_dev);
return 0;
}