| Message ID | 20210309031028.97385-1-xiangxia.m.yue@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sock: simplify tw proto registration | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: linmiaohe@huawei.com daniel@iogearbox.net willemb@google.com edumazet@google.com pabeni@redhat.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
On Mon, Mar 8, 2021 at 7:15 PM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > Introduce a new function twsk_prot_init, inspired by > req_prot_init, to simplify the "proto_register" function. > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > net/core/sock.c | 44 ++++++++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 16 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 0ed98f20448a..610de4295101 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot) > return 0; > } > > +static int twsk_prot_init(const struct proto *prot) > +{ > + struct timewait_sock_ops *twsk_prot = prot->twsk_prot; > + > + if (!twsk_prot) > + return 0; > + > + twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", > + prot->name); > + if (!twsk_prot->twsk_slab_name) > + return -ENOMEM; > + > + twsk_prot->twsk_slab = > + kmem_cache_create(twsk_prot->twsk_slab_name, > + twsk_prot->twsk_obj_size, 0, > + SLAB_ACCOUNT | prot->slab_flags, > + NULL); > + if (!twsk_prot->twsk_slab) { > + pr_crit("%s: Can't create timewait sock SLAB cache!\n", > + prot->name); > + return -ENOMEM; > + } > + > + return 0; > +} > + So one issue here is that you have two returns but they both have the same error clean-up outside of the function. It might make more sense to look at freeing the kasprintf if the slab allocation fails and then using the out_free_request_sock_slab jump label below if the slab allocation failed. > int proto_register(struct proto *prot, int alloc_slab) > { > int ret = -ENOBUFS; > @@ -3496,22 +3522,8 @@ int proto_register(struct proto *prot, int alloc_slab) > if (req_prot_init(prot)) > goto out_free_request_sock_slab; > > - if (prot->twsk_prot != NULL) { > - prot->twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", prot->name); > - > - if (prot->twsk_prot->twsk_slab_name == NULL) > - goto out_free_request_sock_slab; > - > - prot->twsk_prot->twsk_slab = > - kmem_cache_create(prot->twsk_prot->twsk_slab_name, > - prot->twsk_prot->twsk_obj_size, > - 0, > - SLAB_ACCOUNT | > - prot->slab_flags, > - NULL); > - if (prot->twsk_prot->twsk_slab == NULL) > - goto out_free_timewait_sock_slab; > - } > + if (twsk_prot_init(prot)) > + goto out_free_timewait_sock_slab; So assuming the code above takes care of freeing the slab name in case of slab allocation failure then this would be better off jumping to out_free_request_sock_slab. > } > > mutex_lock(&proto_list_mutex); > -- > 2.27.0 >
On Wed, Mar 10, 2021 at 1:39 AM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Mon, Mar 8, 2021 at 7:15 PM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > Introduce a new function twsk_prot_init, inspired by > > req_prot_init, to simplify the "proto_register" function. > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > net/core/sock.c | 44 ++++++++++++++++++++++++++++---------------- > > 1 file changed, 28 insertions(+), 16 deletions(-) > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > index 0ed98f20448a..610de4295101 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot) > > return 0; > > } > > > > +static int twsk_prot_init(const struct proto *prot) > > +{ > > + struct timewait_sock_ops *twsk_prot = prot->twsk_prot; > > + > > + if (!twsk_prot) > > + return 0; > > + > > + twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", > > + prot->name); > > + if (!twsk_prot->twsk_slab_name) > > + return -ENOMEM; > > + > > + twsk_prot->twsk_slab = > > + kmem_cache_create(twsk_prot->twsk_slab_name, > > + twsk_prot->twsk_obj_size, 0, > > + SLAB_ACCOUNT | prot->slab_flags, > > + NULL); > > + if (!twsk_prot->twsk_slab) { > > + pr_crit("%s: Can't create timewait sock SLAB cache!\n", > > + prot->name); > > + return -ENOMEM; > > + } > > + > > + return 0; > > +} > > + > > So one issue here is that you have two returns but they both have the > same error clean-up outside of the function. It might make more sense > to look at freeing the kasprintf if the slab allocation fails and then > using the out_free_request_sock_slab jump label below if the slab > allocation failed. Hi, thanks for your review. if twsk_prot_init failed, (kasprintf, or slab alloc), we will invoke the tw_prot_cleanup() to clean up the sources allocated. 1. kfree(twsk_prot->twsk_slab_name); // if name is NULL, kfree() will return directly 2. kmem_cache_destroy(twsk_prot->twsk_slab); // if slab is NULL, kmem_cache_destroy() will return directly too. so we don't care what err in twsk_prot_init(). and req_prot_cleanup() will clean up all sources allocated for req_prot_init(). > > int proto_register(struct proto *prot, int alloc_slab) > > { > > int ret = -ENOBUFS; > > @@ -3496,22 +3522,8 @@ int proto_register(struct proto *prot, int alloc_slab) > > if (req_prot_init(prot)) > > goto out_free_request_sock_slab; > > > > - if (prot->twsk_prot != NULL) { > > - prot->twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", prot->name); > > - > > - if (prot->twsk_prot->twsk_slab_name == NULL) > > - goto out_free_request_sock_slab; > > - > > - prot->twsk_prot->twsk_slab = > > - kmem_cache_create(prot->twsk_prot->twsk_slab_name, > > - prot->twsk_prot->twsk_obj_size, > > - 0, > > - SLAB_ACCOUNT | > > - prot->slab_flags, > > - NULL); > > - if (prot->twsk_prot->twsk_slab == NULL) > > - goto out_free_timewait_sock_slab; > > - } > > + if (twsk_prot_init(prot)) > > + goto out_free_timewait_sock_slab; > > So assuming the code above takes care of freeing the slab name in case > of slab allocation failure then this would be better off jumping to > out_free_request_sock_slab. > > > } > > > > mutex_lock(&proto_list_mutex); > > -- > > 2.27.0 > >
On Tue, Mar 9, 2021 at 5:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Wed, Mar 10, 2021 at 1:39 AM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Mon, Mar 8, 2021 at 7:15 PM <xiangxia.m.yue@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > Introduce a new function twsk_prot_init, inspired by > > > req_prot_init, to simplify the "proto_register" function. > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > --- > > > net/core/sock.c | 44 ++++++++++++++++++++++++++++---------------- > > > 1 file changed, 28 insertions(+), 16 deletions(-) > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > index 0ed98f20448a..610de4295101 100644 > > > --- a/net/core/sock.c > > > +++ b/net/core/sock.c > > > @@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot) > > > return 0; > > > } > > > > > > +static int twsk_prot_init(const struct proto *prot) > > > +{ > > > + struct timewait_sock_ops *twsk_prot = prot->twsk_prot; > > > + > > > + if (!twsk_prot) > > > + return 0; > > > + > > > + twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", > > > + prot->name); > > > + if (!twsk_prot->twsk_slab_name) > > > + return -ENOMEM; > > > + > > > + twsk_prot->twsk_slab = > > > + kmem_cache_create(twsk_prot->twsk_slab_name, > > > + twsk_prot->twsk_obj_size, 0, > > > + SLAB_ACCOUNT | prot->slab_flags, > > > + NULL); > > > + if (!twsk_prot->twsk_slab) { > > > + pr_crit("%s: Can't create timewait sock SLAB cache!\n", > > > + prot->name); > > > + return -ENOMEM; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > > So one issue here is that you have two returns but they both have the > > same error clean-up outside of the function. It might make more sense > > to look at freeing the kasprintf if the slab allocation fails and then > > using the out_free_request_sock_slab jump label below if the slab > > allocation failed. > Hi, thanks for your review. > if twsk_prot_init failed, (kasprintf, or slab alloc), we will invoke > the tw_prot_cleanup() to clean up > the sources allocated. > 1. kfree(twsk_prot->twsk_slab_name); // if name is NULL, kfree() will > return directly > 2. kmem_cache_destroy(twsk_prot->twsk_slab); // if slab is NULL, > kmem_cache_destroy() will return directly too. > so we don't care what err in twsk_prot_init(). > > and req_prot_cleanup() will clean up all sources allocated for req_prot_init(). I see. Okay so the expectation is that tw_prot_cleanup will take care of a partially initialized timewait_sock_ops. With that being the case the one change I would ask you to make would be to look at moving the function up so it is just below tw_prot_cleanup so it is obvious that the two are meant to be paired rather than placing it after req_prot_init. Otherwise the patch set itself looks good to me. Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
On Wed, Mar 10, 2021 at 10:42 AM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Tue, Mar 9, 2021 at 5:48 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Wed, Mar 10, 2021 at 1:39 AM Alexander Duyck > > <alexander.duyck@gmail.com> wrote: > > > > > > On Mon, Mar 8, 2021 at 7:15 PM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > > > Introduce a new function twsk_prot_init, inspired by > > > > req_prot_init, to simplify the "proto_register" function. > > > > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > --- > > > > net/core/sock.c | 44 ++++++++++++++++++++++++++++---------------- > > > > 1 file changed, 28 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/net/core/sock.c b/net/core/sock.c > > > > index 0ed98f20448a..610de4295101 100644 > > > > --- a/net/core/sock.c > > > > +++ b/net/core/sock.c > > > > @@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot) > > > > return 0; > > > > } > > > > > > > > +static int twsk_prot_init(const struct proto *prot) > > > > +{ > > > > + struct timewait_sock_ops *twsk_prot = prot->twsk_prot; > > > > + > > > > + if (!twsk_prot) > > > > + return 0; > > > > + > > > > + twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", > > > > + prot->name); > > > > + if (!twsk_prot->twsk_slab_name) > > > > + return -ENOMEM; > > > > + > > > > + twsk_prot->twsk_slab = > > > > + kmem_cache_create(twsk_prot->twsk_slab_name, > > > > + twsk_prot->twsk_obj_size, 0, > > > > + SLAB_ACCOUNT | prot->slab_flags, > > > > + NULL); > > > > + if (!twsk_prot->twsk_slab) { > > > > + pr_crit("%s: Can't create timewait sock SLAB cache!\n", > > > > + prot->name); > > > > + return -ENOMEM; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > So one issue here is that you have two returns but they both have the > > > same error clean-up outside of the function. It might make more sense > > > to look at freeing the kasprintf if the slab allocation fails and then > > > using the out_free_request_sock_slab jump label below if the slab > > > allocation failed. > > Hi, thanks for your review. > > if twsk_prot_init failed, (kasprintf, or slab alloc), we will invoke > > the tw_prot_cleanup() to clean up > > the sources allocated. > > 1. kfree(twsk_prot->twsk_slab_name); // if name is NULL, kfree() will > > return directly > > 2. kmem_cache_destroy(twsk_prot->twsk_slab); // if slab is NULL, > > kmem_cache_destroy() will return directly too. > > so we don't care what err in twsk_prot_init(). > > > > and req_prot_cleanup() will clean up all sources allocated for req_prot_init(). > > I see. Okay so the expectation is that tw_prot_cleanup will take care > of a partially initialized timewait_sock_ops. > > With that being the case the one change I would ask you to make would > be to look at moving the function up so it is just below > tw_prot_cleanup so it is obvious that the two are meant to be paired > rather than placing it after req_prot_init. Thanks, will be changed in v2 and change the new function name from twsk_prot_init to tw_prot_init (tw_prot_cleanup). > Otherwise the patch set itself looks good to me. > > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
diff --git a/net/core/sock.c b/net/core/sock.c index 0ed98f20448a..610de4295101 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -3475,6 +3475,32 @@ static int req_prot_init(const struct proto *prot) return 0; } +static int twsk_prot_init(const struct proto *prot) +{ + struct timewait_sock_ops *twsk_prot = prot->twsk_prot; + + if (!twsk_prot) + return 0; + + twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", + prot->name); + if (!twsk_prot->twsk_slab_name) + return -ENOMEM; + + twsk_prot->twsk_slab = + kmem_cache_create(twsk_prot->twsk_slab_name, + twsk_prot->twsk_obj_size, 0, + SLAB_ACCOUNT | prot->slab_flags, + NULL); + if (!twsk_prot->twsk_slab) { + pr_crit("%s: Can't create timewait sock SLAB cache!\n", + prot->name); + return -ENOMEM; + } + + return 0; +} + int proto_register(struct proto *prot, int alloc_slab) { int ret = -ENOBUFS; @@ -3496,22 +3522,8 @@ int proto_register(struct proto *prot, int alloc_slab) if (req_prot_init(prot)) goto out_free_request_sock_slab; - if (prot->twsk_prot != NULL) { - prot->twsk_prot->twsk_slab_name = kasprintf(GFP_KERNEL, "tw_sock_%s", prot->name); - - if (prot->twsk_prot->twsk_slab_name == NULL) - goto out_free_request_sock_slab; - - prot->twsk_prot->twsk_slab = - kmem_cache_create(prot->twsk_prot->twsk_slab_name, - prot->twsk_prot->twsk_obj_size, - 0, - SLAB_ACCOUNT | - prot->slab_flags, - NULL); - if (prot->twsk_prot->twsk_slab == NULL) - goto out_free_timewait_sock_slab; - } + if (twsk_prot_init(prot)) + goto out_free_timewait_sock_slab; } mutex_lock(&proto_list_mutex);