| Message ID | 8ba404fd-7f41-44a9-9869-84f3af18fb46@web.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | net/smc: Adjustments for two function implementations | expand |
On 2023/12/31 22:55, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 31 Dec 2023 15:48:45 +0100 > > A few update suggestions were taken into account > from static source code analysis. > > Markus Elfring (2): > Return directly after a failed kzalloc() in smc_fill_gid_list() > Improve exception handling in smc_llc_cli_add_link_invite() > > net/smc/af_smc.c | 2 +- > net/smc/smc_llc.c | 15 +++++++-------- > 2 files changed, 8 insertions(+), 9 deletions(-) > > -- > 2.43.0 Hi Markus. I see you want to fix the kfree(NULL) issues in these two patches. But I am wondering if this is necessary, since kfree() can handle NULL correctly. /** * kfree - free previously allocated memory * @object: pointer returned by kmalloc() or kmem_cache_alloc() * * If @object is NULL, no operation is performed. */ void kfree(const void *object) { struct folio *folio; struct slab *slab; struct kmem_cache *s; trace_kfree(_RET_IP_, object); if (unlikely(ZERO_OR_NULL_PTR(object))) return; folio = virt_to_folio(object); if (unlikely(!folio_test_slab(folio))) { free_large_kmalloc(folio, (void *)object); return; } slab = folio_slab(folio); s = slab->slab_cache; __kmem_cache_free(s, (void *)object, _RET_IP_); } EXPORT_SYMBOL(kfree); Thanks, Wen Gu
On Tue, Jan 02, 2024 at 04:13:09PM +0800, Wen Gu wrote: > > > On 2023/12/31 22:55, Markus Elfring wrote: > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Sun, 31 Dec 2023 15:48:45 +0100 > > > > A few update suggestions were taken into account > > from static source code analysis. > > > > Markus Elfring (2): > > Return directly after a failed kzalloc() in smc_fill_gid_list() > > Improve exception handling in smc_llc_cli_add_link_invite() > > > > net/smc/af_smc.c | 2 +- > > net/smc/smc_llc.c | 15 +++++++-------- > > 2 files changed, 8 insertions(+), 9 deletions(-) > > > > -- > > 2.43.0 > > Hi Markus. I see you want to fix the kfree(NULL) issues in these two patches. > > But I am wondering if this is necessary, since kfree() can handle NULL correctly. I think the key point is that there are no necessary to call kfree() if we can avoid this in normal logic. Tony Lu
… >> A few update suggestions were taken into account >> from static source code analysis. … >> Return directly after a failed kzalloc() in smc_fill_gid_list() >> Improve exception handling in smc_llc_cli_add_link_invite() >> >> net/smc/af_smc.c | 2 +- >> net/smc/smc_llc.c | 15 +++++++-------- >> 2 files changed, 8 insertions(+), 9 deletions(-) … > I see you want to fix the kfree(NULL) issues in these two patches. I propose to avoid redundant function calls at various source code places. > But I am wondering if this is necessary, since kfree() can handle NULL correctly. Would you prefer only required data processing in affected function implementations? Regards, Markus
On 2024/1/2 16:51, Markus Elfring wrote: > … >>> A few update suggestions were taken into account >>> from static source code analysis. > … >>> Return directly after a failed kzalloc() in smc_fill_gid_list() >>> Improve exception handling in smc_llc_cli_add_link_invite() >>> >>> net/smc/af_smc.c | 2 +- >>> net/smc/smc_llc.c | 15 +++++++-------- >>> 2 files changed, 8 insertions(+), 9 deletions(-) > … >> I see you want to fix the kfree(NULL) issues in these two patches. > > I propose to avoid redundant function calls at various source code places. > > >> But I am wondering if this is necessary, since kfree() can handle NULL correctly. > > Would you prefer only required data processing in affected function implementations? > Thank you Markus. I understood that you want to avoid redundant function calls. But it is not very attractive to me since the calls occur on low-frequency paths or unlikely condition, resulting in limited performance loss and the current kfree() usage is fine and common. So what is the benfit? I noticed that some other discussions are on-going. It seems like you are trying to change other similiar places. Let's collect more opinions. https://lore.kernel.org/netdev/828bb442-29d0-4bb8-b90d-f200bdd4faf6@web.de/ https://lore.kernel.org/netdev/90679f69-951c-47b3-b86f-75fd9fde3da3@web.de/ https://lore.kernel.org/netdev/dc0a1c9d-ceca-473d-9ad5-89b59e6af2e7@web.de/ https://lore.kernel.org/netdev/cde82080-c715-473c-97ac-6ef66bba6d64@web.de/ Thanks. > Regards, > Markus
> But it is not very attractive to me since the calls occur on low-frequency paths > or unlikely condition, resulting in limited performance loss and the current > kfree() usage is fine and common. The prioritisation of development activities influences progress in related areas. > So what is the benfit? * Source code clarity * Nicer run time characteristics See also: https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources > I noticed that some other discussions are on-going. It seems like you are trying > to change other similiar places. I would appreciate if improvements can be achieved also for similarly affected software components. Regards, Markus
On 31.12.23 15:55, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 31 Dec 2023 15:48:45 +0100 > > A few update suggestions were taken into account > from static source code analysis. > > Markus Elfring (2): > Return directly after a failed kzalloc() in smc_fill_gid_list() > Improve exception handling in smc_llc_cli_add_link_invite() > > net/smc/af_smc.c | 2 +- > net/smc/smc_llc.c | 15 +++++++-------- > 2 files changed, 8 insertions(+), 9 deletions(-) > > -- > 2.43.0 > Hi Markus, Thank you for trying to improve our code! However, I'm on the same page with Wen Gu. I could not see the necessity of the patches. BTW, if you want to send fix patches, please provide the error messages you met, the procedure of reproducing the issue and the correspoinding commit messages. If you want to send feature patches, I'd like to see a well thought-out patch or patch series. E.g. In our component, the kfree(NULL) issue doesn't only occur in the positions where you mentioned in the patch series, also somewhere else. I would be grateful if all of them would be cleaned up, not just some pieces. Thanks, Wenjia
… >> Return directly after a failed kzalloc() in smc_fill_gid_list() >> Improve exception handling in smc_llc_cli_add_link_invite() >> >> net/smc/af_smc.c | 2 +- >> net/smc/smc_llc.c | 15 +++++++-------- >> 2 files changed, 8 insertions(+), 9 deletions(-) … > However, I'm on the same page with Wen Gu. I could not see the necessity of the patches. > BTW, if you want to send fix patches, I obviously propose to adjust specific implementation details. > please provide the error messages you met, This development concern does not apply here. > the procedure of reproducing the issue and the correspoinding commit messages. Would you like to extend the usage of source code analysis tools? > If you want to send feature patches, I'd like to see a well thought-out patch or patch series. I presented some thoughts for special transformation patterns on several software components. > E.g. In our component, the kfree(NULL) issue doesn't only occur in the positions where you mentioned in the patch series, also somewhere else. Does your feedback indicate that you would support the avoidance of such a special function call at more places? > I would be grateful if all of them would be cleaned up, not just some pieces. Do you find my patch series too small for the mentioned Linux module at the moment? Regards, Markus
On Tue, Jan 02, 2024 at 07:33:18PM +0800, Wen Gu wrote: > > > On 2024/1/2 16:51, Markus Elfring wrote: > > … > > > > A few update suggestions were taken into account > > > > from static source code analysis. > > … > > > > Return directly after a failed kzalloc() in smc_fill_gid_list() > > > > Improve exception handling in smc_llc_cli_add_link_invite() > > > > > > > > net/smc/af_smc.c | 2 +- > > > > net/smc/smc_llc.c | 15 +++++++-------- > > > > 2 files changed, 8 insertions(+), 9 deletions(-) > > … > > > I see you want to fix the kfree(NULL) issues in these two patches. > > > > I propose to avoid redundant function calls at various source code places. > > > > > > > But I am wondering if this is necessary, since kfree() can handle NULL correctly. > > > > Would you prefer only required data processing in affected function implementations? > > > > Thank you Markus. I understood that you want to avoid redundant function calls. > > But it is not very attractive to me since the calls occur on low-frequency paths > or unlikely condition, resulting in limited performance loss and the current > kfree() usage is fine and common. So what is the benfit? > > I noticed that some other discussions are on-going. It seems like you are trying > to change other similiar places. Let's collect more opinions. > > https://lore.kernel.org/netdev/828bb442-29d0-4bb8-b90d-f200bdd4faf6@web.de/ > https://lore.kernel.org/netdev/90679f69-951c-47b3-b86f-75fd9fde3da3@web.de/ > https://lore.kernel.org/netdev/dc0a1c9d-ceca-473d-9ad5-89b59e6af2e7@web.de/ > https://lore.kernel.org/netdev/cde82080-c715-473c-97ac-6ef66bba6d64@web.de/ As as been explained to Markus many times recently, calling kfree(NULL) is not only perfectly fine, it is the preferred way of handling things. Markus, please stop posting patches of this nature to Netdev.
From: Markus Elfring <elfring@users.sourceforge.net> Date: Sun, 31 Dec 2023 15:48:45 +0100 A few update suggestions were taken into account from static source code analysis. Markus Elfring (2): Return directly after a failed kzalloc() in smc_fill_gid_list() Improve exception handling in smc_llc_cli_add_link_invite() net/smc/af_smc.c | 2 +- net/smc/smc_llc.c | 15 +++++++-------- 2 files changed, 8 insertions(+), 9 deletions(-) -- 2.43.0