| Message ID | 4a096ba9cf981a588aa87235bb91e933ee162b3d.1695542544.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef35bed6fad6eda8de0277eda77803c748f306d1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] udp_tunnel: Use flex array to simplify code | expand |
Christophe JAILLET wrote: > 'n_tables' is small, UDP_TUNNEL_NIC_MAX_TABLES = 4 as a maximum. So there > is no real point to allocate the 'entries' pointers array with a dedicate > memory allocation. > > Using a flexible array for struct udp_tunnel_nic->entries avoids the > overhead of an additional memory allocation. > > This also saves an indirection when the array is accessed. > > Finally, __counted_by() can be used for run-time bounds checking if > configured and supported by the compiler. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > net/ipv4/udp_tunnel_nic.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c > index 029219749785..b6d2d16189c0 100644 > --- a/net/ipv4/udp_tunnel_nic.c > +++ b/net/ipv4/udp_tunnel_nic.c > @@ -47,7 +47,7 @@ struct udp_tunnel_nic { > > unsigned int n_tables; > unsigned long missed; > - struct udp_tunnel_nic_table_entry **entries; > + struct udp_tunnel_nic_table_entry *entries[] __counted_by(n_tables); > }; > > /* We ensure all work structs are done using driver state, but not the code. > @@ -725,16 +725,12 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, > struct udp_tunnel_nic *utn; > unsigned int i; > > - utn = kzalloc(sizeof(*utn), GFP_KERNEL); > + utn = kzalloc(struct_size(utn, entries, n_tables), GFP_KERNEL); > if (!utn) > return NULL; > utn->n_tables = n_tables; Should utn->n_tables be initialized before first use of struct_size(utn, entries, n_tables)?
Le 24/09/2023 à 18:00, Willem de Bruijn a écrit : > Christophe JAILLET wrote: >> 'n_tables' is small, UDP_TUNNEL_NIC_MAX_TABLES = 4 as a maximum. So there >> is no real point to allocate the 'entries' pointers array with a dedicate >> memory allocation. >> >> Using a flexible array for struct udp_tunnel_nic->entries avoids the >> overhead of an additional memory allocation. >> >> This also saves an indirection when the array is accessed. >> >> Finally, __counted_by() can be used for run-time bounds checking if >> configured and supported by the compiler. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> net/ipv4/udp_tunnel_nic.c | 11 ++--------- >> 1 file changed, 2 insertions(+), 9 deletions(-) >> >> diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c >> index 029219749785..b6d2d16189c0 100644 >> --- a/net/ipv4/udp_tunnel_nic.c >> +++ b/net/ipv4/udp_tunnel_nic.c >> @@ -47,7 +47,7 @@ struct udp_tunnel_nic { >> >> unsigned int n_tables; >> unsigned long missed; >> - struct udp_tunnel_nic_table_entry **entries; >> + struct udp_tunnel_nic_table_entry *entries[] __counted_by(n_tables); >> }; >> >> /* We ensure all work structs are done using driver state, but not the code. >> @@ -725,16 +725,12 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, >> struct udp_tunnel_nic *utn; >> unsigned int i; >> >> - utn = kzalloc(sizeof(*utn), GFP_KERNEL); >> + utn = kzalloc(struct_size(utn, entries, n_tables), GFP_KERNEL); >> if (!utn) >> return NULL; >> utn->n_tables = n_tables; > > Should utn->n_tables be initialized before first use of > struct_size(utn, entries, n_tables)? > It can't be. struct_size() is used to compute the memory size to allocate. Before the kzalloc() call, utn does not exist, so we can't write anything to utn->n_tables. It is undefined at this point. It is initialized the line just after, after the allocation, but before any use. CJ
On Mon, Sep 25, 2023 at 6:26 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > Le 24/09/2023 à 18:00, Willem de Bruijn a écrit : > > Christophe JAILLET wrote: > >> 'n_tables' is small, UDP_TUNNEL_NIC_MAX_TABLES = 4 as a maximum. So there > >> is no real point to allocate the 'entries' pointers array with a dedicate > >> memory allocation. > >> > >> Using a flexible array for struct udp_tunnel_nic->entries avoids the > >> overhead of an additional memory allocation. > >> > >> This also saves an indirection when the array is accessed. > >> > >> Finally, __counted_by() can be used for run-time bounds checking if > >> configured and supported by the compiler. > >> > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Reviewed-by: Willem de Bruijn <willemb@google.com> > >> --- > >> net/ipv4/udp_tunnel_nic.c | 11 ++--------- > >> 1 file changed, 2 insertions(+), 9 deletions(-) > >> > >> diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c > >> index 029219749785..b6d2d16189c0 100644 > >> --- a/net/ipv4/udp_tunnel_nic.c > >> +++ b/net/ipv4/udp_tunnel_nic.c > >> @@ -47,7 +47,7 @@ struct udp_tunnel_nic { > >> > >> unsigned int n_tables; > >> unsigned long missed; > >> - struct udp_tunnel_nic_table_entry **entries; > >> + struct udp_tunnel_nic_table_entry *entries[] __counted_by(n_tables); > >> }; > >> > >> /* We ensure all work structs are done using driver state, but not the code. > >> @@ -725,16 +725,12 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, > >> struct udp_tunnel_nic *utn; > >> unsigned int i; > >> > >> - utn = kzalloc(sizeof(*utn), GFP_KERNEL); > >> + utn = kzalloc(struct_size(utn, entries, n_tables), GFP_KERNEL); > >> if (!utn) > >> return NULL; > >> utn->n_tables = n_tables; > > > > Should utn->n_tables be initialized before first use of > > struct_size(utn, entries, n_tables)? > > > > It can't be. > struct_size() is used to compute the memory size to allocate. > > Before the kzalloc() call, utn does not exist, so we can't write > anything to utn->n_tables. It is undefined at this point. > > It is initialized the line just after, after the allocation, but before > any use. Of course, sorry. I confused __counted_by's reference to a field member in the struct, with the normal argument passing of struct_size and flex_array_size.
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sun, 24 Sep 2023 10:03:07 +0200 you wrote: > 'n_tables' is small, UDP_TUNNEL_NIC_MAX_TABLES = 4 as a maximum. So there > is no real point to allocate the 'entries' pointers array with a dedicate > memory allocation. > > Using a flexible array for struct udp_tunnel_nic->entries avoids the > overhead of an additional memory allocation. > > [...] Here is the summary with links: - [net-next] udp_tunnel: Use flex array to simplify code https://git.kernel.org/netdev/net-next/c/ef35bed6fad6 You are awesome, thank you!
diff --git a/net/ipv4/udp_tunnel_nic.c b/net/ipv4/udp_tunnel_nic.c index 029219749785..b6d2d16189c0 100644 --- a/net/ipv4/udp_tunnel_nic.c +++ b/net/ipv4/udp_tunnel_nic.c @@ -47,7 +47,7 @@ struct udp_tunnel_nic { unsigned int n_tables; unsigned long missed; - struct udp_tunnel_nic_table_entry **entries; + struct udp_tunnel_nic_table_entry *entries[] __counted_by(n_tables); }; /* We ensure all work structs are done using driver state, but not the code. @@ -725,16 +725,12 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, struct udp_tunnel_nic *utn; unsigned int i; - utn = kzalloc(sizeof(*utn), GFP_KERNEL); + utn = kzalloc(struct_size(utn, entries, n_tables), GFP_KERNEL); if (!utn) return NULL; utn->n_tables = n_tables; INIT_WORK(&utn->work, udp_tunnel_nic_device_sync_work); - utn->entries = kmalloc_array(n_tables, sizeof(void *), GFP_KERNEL); - if (!utn->entries) - goto err_free_utn; - for (i = 0; i < n_tables; i++) { utn->entries[i] = kcalloc(info->tables[i].n_entries, sizeof(*utn->entries[i]), GFP_KERNEL); @@ -747,8 +743,6 @@ udp_tunnel_nic_alloc(const struct udp_tunnel_nic_info *info, err_free_prev_entries: while (i--) kfree(utn->entries[i]); - kfree(utn->entries); -err_free_utn: kfree(utn); return NULL; } @@ -759,7 +753,6 @@ static void udp_tunnel_nic_free(struct udp_tunnel_nic *utn) for (i = 0; i < utn->n_tables; i++) kfree(utn->entries[i]); - kfree(utn->entries); kfree(utn); }
'n_tables' is small, UDP_TUNNEL_NIC_MAX_TABLES = 4 as a maximum. So there is no real point to allocate the 'entries' pointers array with a dedicate memory allocation. Using a flexible array for struct udp_tunnel_nic->entries avoids the overhead of an additional memory allocation. This also saves an indirection when the array is accessed. Finally, __counted_by() can be used for run-time bounds checking if configured and supported by the compiler. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- net/ipv4/udp_tunnel_nic.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)