diff mbox series

ethdev: avoid unregistering a non-allocated callback

Message ID 20210713131714.964500-1-thomas@monjalon.net (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series ethdev: avoid unregistering a non-allocated callback |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot success github build: passed
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-testing fail Testing issues
ci/iol-intel-Performance fail Performance Testing issues

Commit Message

Thomas Monjalon July 13, 2021, 1:17 p.m. UTC 
  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(-)

Comments

Andrew Rybchenko July 13, 2021, 1:21 p.m. UTC | #1 
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>
Matan Azrad July 13, 2021, 1:42 p.m. UTC | #2 
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(&eth_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
Thomas Monjalon July 14, 2021, 11:42 a.m. UTC | #3 
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(&eth_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.
Matan Azrad July 14, 2021, 2:16 p.m. UTC | #4 
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(&eth_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
Thomas Monjalon July 14, 2021, 2:42 p.m. UTC | #5 
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(&eth_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.
Ferruh Yigit July 15, 2021, 9:06 a.m. UTC | #6 
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(&eth_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.
Thomas Monjalon Sept. 26, 2022, 2:36 p.m. UTC | #7 
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(&eth_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.
diff mbox series

Patch

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(&eth_dev_cb_lock);
-				rte_eth_dev_callback_unregister(port_id, event,
-								cb_fn, cb_arg);
 				return -ENOMEM;
 			}