* [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
@ 2021-07-13 13:17 Thomas Monjalon
2021-07-13 13:21 ` Andrew Rybchenko
2021-07-13 13:42 ` Matan Azrad
0 siblings, 2 replies; 8+ messages in thread
From: Thomas Monjalon @ 2021-07-13 13:17 UTC (permalink / raw)
To: dev; +Cc: stable, Ferruh Yigit, Andrew Rybchenko, Matan Azrad
When registering a new event callback, if allocation fails,
there is no need for unregistering the callback,
because it is not registered.
Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all ports")
Cc: stable@dpdk.org
Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
lib/ethdev/rte_ethdev.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9d95cd11e1..1731854628 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t port_id,
user_cb, next);
} else {
rte_spinlock_unlock(ð_dev_cb_lock);
- rte_eth_dev_callback_unregister(port_id, event,
- cb_fn, cb_arg);
return -ENOMEM;
}
--
2.31.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-13 13:17 [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback Thomas Monjalon
@ 2021-07-13 13:21 ` Andrew Rybchenko
2021-07-13 13:42 ` Matan Azrad
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Rybchenko @ 2021-07-13 13:21 UTC (permalink / raw)
To: Thomas Monjalon, dev; +Cc: stable, Ferruh Yigit, Matan Azrad
On 7/13/21 4:17 PM, Thomas Monjalon wrote:
> When registering a new event callback, if allocation fails,
> there is no need for unregistering the callback,
> because it is not registered.
>
> Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all ports")
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-13 13:17 [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback Thomas Monjalon
2021-07-13 13:21 ` Andrew Rybchenko
@ 2021-07-13 13:42 ` Matan Azrad
2021-07-14 11:42 ` Thomas Monjalon
1 sibling, 1 reply; 8+ messages in thread
From: Matan Azrad @ 2021-07-13 13:42 UTC (permalink / raw)
To: NBU-Contact-Thomas Monjalon, dev; +Cc: stable, Ferruh Yigit, Andrew Rybchenko
Hi Thomas
From: Thomas Monjalon
> When registering a new event callback, if allocation fails, there is no need for
> unregistering the callback, because it is not registered.
>
> Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all ports")
> Cc: stable@dpdk.org
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> lib/ethdev/rte_ethdev.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> 9d95cd11e1..1731854628 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t port_id,
> user_cb, next);
> } else {
> rte_spinlock_unlock(ð_dev_cb_lock);
> - rte_eth_dev_callback_unregister(port_id, event,
> - cb_fn, cb_arg);
Please pay attention to the case of port_id=RTE_ETH_ALL where the user wants to register the event for all the ports.
In this case, when a failure happens for one of the ports, this unregister call cleans the callback from all the ports.
> return -ENOMEM;
> }
>
> --
> 2.31.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-13 13:42 ` Matan Azrad
@ 2021-07-14 11:42 ` Thomas Monjalon
2021-07-14 14:16 ` Matan Azrad
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-07-14 11:42 UTC (permalink / raw)
To: Matan Azrad; +Cc: dev, Ferruh Yigit, Andrew Rybchenko
13/07/2021 15:42, Matan Azrad:
> Hi Thomas
>
> From: Thomas Monjalon
> > When registering a new event callback, if allocation fails, there is no need for
> > unregistering the callback, because it is not registered.
> >
> > Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all ports")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> > lib/ethdev/rte_ethdev.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> > 9d95cd11e1..1731854628 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t port_id,
> > user_cb, next);
> > } else {
> > rte_spinlock_unlock(ð_dev_cb_lock);
> > - rte_eth_dev_callback_unregister(port_id, event,
> > - cb_fn, cb_arg);
>
> Please pay attention to the case of port_id=RTE_ETH_ALL where the user wants to register the event for all the ports.
>
> In this case, when a failure happens for one of the ports, this unregister call cleans the callback from all the ports.
Yes I missed it. Now I better understand the intent, thanks.
Next question: do we really want to rollback already registered ports?
Anyway, if we are out of memory, I think it is better not doing more operations.
There can be various opinions on this topic, please give yours.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-14 11:42 ` Thomas Monjalon
@ 2021-07-14 14:16 ` Matan Azrad
2021-07-14 14:42 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Matan Azrad @ 2021-07-14 14:16 UTC (permalink / raw)
To: NBU-Contact-Thomas Monjalon; +Cc: dev, Ferruh Yigit, Andrew Rybchenko
From: Thomas Monjalon
> 13/07/2021 15:42, Matan Azrad:
> > Hi Thomas
> >
> > From: Thomas Monjalon
> > > When registering a new event callback, if allocation fails, there is
> > > no need for unregistering the callback, because it is not registered.
> > >
> > > Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all
> > > ports")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> > > lib/ethdev/rte_ethdev.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
> > > 9d95cd11e1..1731854628 100644
> > > --- a/lib/ethdev/rte_ethdev.c
> > > +++ b/lib/ethdev/rte_ethdev.c
> > > @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t
> port_id,
> > > user_cb, next);
> > > } else {
> > > rte_spinlock_unlock(ð_dev_cb_lock);
> > > - rte_eth_dev_callback_unregister(port_id, event,
> > > - cb_fn, cb_arg);
> >
> > Please pay attention to the case of port_id=RTE_ETH_ALL where the user
> wants to register the event for all the ports.
> >
> > In this case, when a failure happens for one of the ports, this unregister call
> cleans the callback from all the ports.
>
> Yes I missed it. Now I better understand the intent, thanks.
>
> Next question: do we really want to rollback already registered ports?
> Anyway, if we are out of memory, I think it is better not doing more
> operations.
> There can be various opinions on this topic, please give yours.
Sure,
I understand that memory error is serious,
Do you think it is a fatal error? If so, maybe we should use rte_exit?
That way or others, I think the behavior should be a convention for all the file functions(at least).
I tend to do cleanup on any error.
Matan
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-14 14:16 ` Matan Azrad
@ 2021-07-14 14:42 ` Thomas Monjalon
2021-07-15 9:06 ` Ferruh Yigit
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Monjalon @ 2021-07-14 14:42 UTC (permalink / raw)
To: Matan Azrad; +Cc: dev, Ferruh Yigit, Andrew Rybchenko
14/07/2021 16:16, Matan Azrad:
> From: Thomas Monjalon
> > 13/07/2021 15:42, Matan Azrad:
> > > From: Thomas Monjalon
> > > > When registering a new event callback, if allocation fails, there is
> > > > no need for unregistering the callback, because it is not registered.
> > > >
> > > > Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all
> > > > ports")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > > ---
> > > > --- a/lib/ethdev/rte_ethdev.c
> > > > +++ b/lib/ethdev/rte_ethdev.c
> > > > @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t
> > > > } else {
> > > > rte_spinlock_unlock(ð_dev_cb_lock);
> > > > - rte_eth_dev_callback_unregister(port_id, event,
> > > > - cb_fn, cb_arg);
> > >
> > > Please pay attention to the case of port_id=RTE_ETH_ALL where the user
> > wants to register the event for all the ports.
> > >
> > > In this case, when a failure happens for one of the ports, this unregister call
> > cleans the callback from all the ports.
> >
> > Yes I missed it. Now I better understand the intent, thanks.
> >
> > Next question: do we really want to rollback already registered ports?
> > Anyway, if we are out of memory, I think it is better not doing more
> > operations.
> > There can be various opinions on this topic, please give yours.
>
> Sure,
> I understand that memory error is serious,
> Do you think it is a fatal error? If so, maybe we should use rte_exit?
We don't call rte_exit in the lib, so the app can do whatever it wants.
> That way or others, I think the behavior should be a convention for all the file functions(at least).
What do you mean "all the file functions"?
> I tend to do cleanup on any error.
I would like to hear opinions from others as well.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-14 14:42 ` Thomas Monjalon
@ 2021-07-15 9:06 ` Ferruh Yigit
2022-09-26 14:36 ` Thomas Monjalon
0 siblings, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2021-07-15 9:06 UTC (permalink / raw)
To: Thomas Monjalon, Matan Azrad; +Cc: dev, Andrew Rybchenko
On 7/14/2021 4:42 PM, Thomas Monjalon wrote:
> 14/07/2021 16:16, Matan Azrad:
>> From: Thomas Monjalon
>>> 13/07/2021 15:42, Matan Azrad:
>>>> From: Thomas Monjalon
>>>>> When registering a new event callback, if allocation fails, there is
>>>>> no need for unregistering the callback, because it is not registered.
>>>>>
>>>>> Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all
>>>>> ports")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>>> ---
>>>>> --- a/lib/ethdev/rte_ethdev.c
>>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>>> @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t
>>>>> } else {
>>>>> rte_spinlock_unlock(ð_dev_cb_lock);
>>>>> - rte_eth_dev_callback_unregister(port_id, event,
>>>>> - cb_fn, cb_arg);
>>>>
>>>> Please pay attention to the case of port_id=RTE_ETH_ALL where the user
>>> wants to register the event for all the ports.
>>>>
>>>> In this case, when a failure happens for one of the ports, this unregister call
>>> cleans the callback from all the ports.
>>>
>>> Yes I missed it. Now I better understand the intent, thanks.
>>>
>>> Next question: do we really want to rollback already registered ports?
>>> Anyway, if we are out of memory, I think it is better not doing more
>>> operations.
>>> There can be various opinions on this topic, please give yours.
>>
>> Sure,
>> I understand that memory error is serious,
>> Do you think it is a fatal error? If so, maybe we should use rte_exit?
>
> We don't call rte_exit in the lib, so the app can do whatever it wants.
>
+1
>> That way or others, I think the behavior should be a convention for all the file functions(at least).
>
> What do you mean "all the file functions"?
>
>> I tend to do cleanup on any error.
>
> I would like to hear opinions from others as well.
>
I also tend to do the cleanup, since API returns error I think application will
be right to think that no callback registered, partially registered callbacks on
error can be confusing.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback
2021-07-15 9:06 ` Ferruh Yigit
@ 2022-09-26 14:36 ` Thomas Monjalon
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2022-09-26 14:36 UTC (permalink / raw)
To: Matan Azrad, Andrew Rybchenko, Ferruh Yigit; +Cc: dev
This patch is abandoned. Current behaviour is kept.
15/07/2021 11:06, Ferruh Yigit:
> On 7/14/2021 4:42 PM, Thomas Monjalon wrote:
> > 14/07/2021 16:16, Matan Azrad:
> >> From: Thomas Monjalon
> >>> 13/07/2021 15:42, Matan Azrad:
> >>>> From: Thomas Monjalon
> >>>>> When registering a new event callback, if allocation fails, there is
> >>>>> no need for unregistering the callback, because it is not registered.
> >>>>>
> >>>>> Fixes: 9ec0b3869d8d ("ethdev: allow event registration for all
> >>>>> ports")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >>>>> ---
> >>>>> --- a/lib/ethdev/rte_ethdev.c
> >>>>> +++ b/lib/ethdev/rte_ethdev.c
> >>>>> @@ -4649,8 +4649,6 @@ rte_eth_dev_callback_register(uint16_t
> >>>>> } else {
> >>>>> rte_spinlock_unlock(ð_dev_cb_lock);
> >>>>> - rte_eth_dev_callback_unregister(port_id, event,
> >>>>> - cb_fn, cb_arg);
> >>>>
> >>>> Please pay attention to the case of port_id=RTE_ETH_ALL where the user
> >>> wants to register the event for all the ports.
> >>>>
> >>>> In this case, when a failure happens for one of the ports, this unregister call
> >>> cleans the callback from all the ports.
> >>>
> >>> Yes I missed it. Now I better understand the intent, thanks.
> >>>
> >>> Next question: do we really want to rollback already registered ports?
> >>> Anyway, if we are out of memory, I think it is better not doing more
> >>> operations.
> >>> There can be various opinions on this topic, please give yours.
> >>
> >> Sure,
> >> I understand that memory error is serious,
> >> Do you think it is a fatal error? If so, maybe we should use rte_exit?
> >
> > We don't call rte_exit in the lib, so the app can do whatever it wants.
> >
>
> +1
>
> >> That way or others, I think the behavior should be a convention for all the file functions(at least).
> >
> > What do you mean "all the file functions"?
> >
> >> I tend to do cleanup on any error.
> >
> > I would like to hear opinions from others as well.
> >
>
> I also tend to do the cleanup, since API returns error I think application will
> be right to think that no callback registered, partially registered callbacks on
> error can be confusing.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-26 14:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 13:17 [dpdk-dev] [PATCH] ethdev: avoid unregistering a non-allocated callback Thomas Monjalon
2021-07-13 13:21 ` Andrew Rybchenko
2021-07-13 13:42 ` Matan Azrad
2021-07-14 11:42 ` Thomas Monjalon
2021-07-14 14:16 ` Matan Azrad
2021-07-14 14:42 ` Thomas Monjalon
2021-07-15 9:06 ` Ferruh Yigit
2022-09-26 14:36 ` Thomas Monjalon
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).