| Message ID | 20210713131714.964500-1-thomas@monjalon.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | ethdev: avoid unregistering a non-allocated callback | expand |
| 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 |
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>
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
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.
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
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.
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.
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.
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; }
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(-)