[PATCH] event: fix warning from useless snprintf

[PATCH] event: fix warning from useless snprintf

[Stephen Hemminger]

With Gcc-14, this warning is generated:
../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
    specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
  263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
      |                 ^

Yet the whole printf to the buf is unnecessary. The type string argument
has never been implemented, and should just be NULL.  Removing the
unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.

Fixes: 5ffb2f142d95 ("event/sw: support event queues")

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/event/sw/iq_chunk.h | 2 --
 drivers/event/sw/sw_evdev.c | 5 +----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/event/sw/iq_chunk.h b/drivers/event/sw/iq_chunk.h
index 7a7a8782e6..e638142dbc 100644
--- a/drivers/event/sw/iq_chunk.h
+++ b/drivers/event/sw/iq_chunk.h
@@ -9,8 +9,6 @@
 #include <stdbool.h>
 #include <rte_eventdev.h>
 
-#define IQ_ROB_NAMESIZE 12
-
 struct __rte_cache_aligned sw_queue_chunk {
 	struct rte_event events[SW_EVS_PER_Q_CHUNK];
 	struct sw_queue_chunk *next;
diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index 1c01b069fe..19a52afc7d 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -228,9 +228,7 @@ qid_init(struct sw_evdev *sw, unsigned int idx, int type,
 		const struct rte_event_queue_conf *queue_conf)
 {
 	unsigned int i;
-	int dev_id = sw->data->dev_id;
 	int socket_id = sw->data->socket_id;
-	char buf[IQ_ROB_NAMESIZE];
 	struct sw_qid *qid = &sw->qids[idx];
 
 	/* Initialize the FID structures to no pinning (-1), and zero packets */
@@ -260,8 +258,7 @@ qid_init(struct sw_evdev *sw, unsigned int idx, int type,
 			goto cleanup;
 		}
 
-		snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
-		qid->reorder_buffer = rte_zmalloc_socket(buf,
+		qid->reorder_buffer = rte_zmalloc_socket(NULL,
 				window_size * sizeof(qid->reorder_buffer[0]),
 				0, socket_id);
 		if (!qid->reorder_buffer) {
-- 
2.43.0

Re: [PATCH] event: fix warning from useless snprintf

[Van Haaren, Harry]

> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, April 24, 2024 4:45 AM
> To: dev@dpdk.org
> Cc: Richardson, Bruce; Stephen Hemminger; Van Haaren, Harry; Jerin Jacob
> Subject: [PATCH] event: fix warning from useless snprintf
> 
> With Gcc-14, this warning is generated:
> ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
>     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
>   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
>       |                 ^
> 
> Yet the whole printf to the buf is unnecessary. The type string argument
> has never been implemented, and should just be NULL.  Removing the
> unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.

I understand that today the "type" value isn't implemented, but across the DPDK codebase it
seems like others are filling in "type" to be some debug-useful name/string. If it was added
in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.


> Fixes: 5ffb2f142d95 ("event/sw: support event queues")
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/event/sw/iq_chunk.h | 2 --
>  drivers/event/sw/sw_evdev.c | 5 +----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/event/sw/iq_chunk.h b/drivers/event/sw/iq_chunk.h
> index 7a7a8782e6..e638142dbc 100644
> --- a/drivers/event/sw/iq_chunk.h
> +++ b/drivers/event/sw/iq_chunk.h
> @@ -9,8 +9,6 @@
>  #include <stdbool.h>
>  #include <rte_eventdev.h>
> 
> -#define IQ_ROB_NAMESIZE 12

We can over-provision the temporary buffer, and solve the problem with minimal changes;
+#define IQ_ROB_NAMESIZE 64

> -
>  struct __rte_cache_aligned sw_queue_chunk {
>         struct rte_event events[SW_EVS_PER_Q_CHUNK];
>         struct sw_queue_chunk *next;
> diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
> index 1c01b069fe..19a52afc7d 100644
> --- a/drivers/event/sw/sw_evdev.c
> +++ b/drivers/event/sw/sw_evdev.c
> @@ -228,9 +228,7 @@ qid_init(struct sw_evdev *sw, unsigned int idx, int type,
>                 const struct rte_event_queue_conf *queue_conf)
>  {
>         unsigned int i;
> -       int dev_id = sw->data->dev_id;
>         int socket_id = sw->data->socket_id;
> -       char buf[IQ_ROB_NAMESIZE];
>         struct sw_qid *qid = &sw->qids[idx];
> 
>         /* Initialize the FID structures to no pinning (-1), and zero packets */
> @@ -260,8 +258,7 @@ qid_init(struct sw_evdev *sw, unsigned int idx, int type,
>                         goto cleanup;
>                 }
> 
> -               snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);

There is a 2nd hidden bug here; the "i" variable iterates the ROB size, and does not represent the QID index.
Changing the "i" to "idx" solves, and prints the expected output instead:
snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, idx);

> -               qid->reorder_buffer = rte_zmalloc_socket(buf,
> +               qid->reorder_buffer = rte_zmalloc_socket(NULL,
>                                 window_size * sizeof(qid->reorder_buffer[0]),
>                                 0, socket_id);
>                 if (!qid->reorder_buffer) {
> --
> 2.43.0

I'm happy to send a patch with the above, if that seems a good solution to you Stephen?

Regards, -Harry

Re: [PATCH] event: fix warning from useless snprintf

[Stephen Hemminger]

On Wed, 24 Apr 2024 08:45:52 +0000
"Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:

> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, April 24, 2024 4:45 AM
> > To: dev@dpdk.org
> > Cc: Richardson, Bruce; Stephen Hemminger; Van Haaren, Harry; Jerin Jacob
> > Subject: [PATCH] event: fix warning from useless snprintf
> > 
> > With Gcc-14, this warning is generated:
> > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> >       |                 ^
> > 
> > Yet the whole printf to the buf is unnecessary. The type string argument
> > has never been implemented, and should just be NULL.  Removing the
> > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.  
> 
> I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> seems like others are filling in "type" to be some debug-useful name/string. If it was added
> in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.

No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

There are better ways of tracking allocations like ASAN.
There are better memory allocators as well which use something different.
 

Re: [PATCH] event: fix warning from useless snprintf

[Van Haaren, Harry]

> ________________________________________
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, April 24, 2024 5:13 PM
> To: Van Haaren, Harry
> Cc: dev@dpdk.org; Richardson, Bruce; Jerin Jacob
> Subject: Re: [PATCH] event: fix warning from useless snprintf
> 
> On Wed, 24 Apr 2024 08:45:52 +0000
> "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
> 
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Wednesday, April 24, 2024 4:45 AM
> > > To: dev@dpdk.org
> > > Cc: Richardson, Bruce; Stephen Hemminger; Van Haaren, Harry; Jerin Jacob
> > > Subject: [PATCH] event: fix warning from useless snprintf
> > >
> > > With Gcc-14, this warning is generated:
> > > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> > >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> > >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> > >       |                 ^
> > >
> > > Yet the whole printf to the buf is unnecessary. The type string argument
> > > has never been implemented, and should just be NULL.  Removing the
> > > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.
> >
> > I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> > seems like others are filling in "type" to be some debug-useful name/string. If it was added
> > in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.
> 
> No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

I agree that YAGNI perhaps applied when designing the APIs, but the "type" parameter is there now...
Should we add a guidance of "when reworking code, always pass NULL as the type parameter to rte_malloc functions" somewhere in the programmers guide, to align community with this "pass NULL for type" initiative?

<snip>

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Re: [PATCH] event: fix warning from useless snprintf

[Stephen Hemminger]

On Wed, 24 Apr 2024 17:12:39 +0000
"Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:

> > ________________________________________
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, April 24, 2024 5:13 PM
> > To: Van Haaren, Harry
> > Cc: dev@dpdk.org; Richardson, Bruce; Jerin Jacob
> > Subject: Re: [PATCH] event: fix warning from useless snprintf
> > 
> > On Wed, 24 Apr 2024 08:45:52 +0000
> > "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
> >   
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Wednesday, April 24, 2024 4:45 AM
> > > > To: dev@dpdk.org
> > > > Cc: Richardson, Bruce; Stephen Hemminger; Van Haaren, Harry; Jerin Jacob
> > > > Subject: [PATCH] event: fix warning from useless snprintf
> > > >
> > > > With Gcc-14, this warning is generated:
> > > > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> > > >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> > > >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> > > >       |                 ^
> > > >
> > > > Yet the whole printf to the buf is unnecessary. The type string argument
> > > > has never been implemented, and should just be NULL.  Removing the
> > > > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.  
> > >
> > > I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> > > seems like others are filling in "type" to be some debug-useful name/string. If it was added
> > > in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.  
> > 
> > No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it  
> 
> I agree that YAGNI perhaps applied when designing the APIs, but the "type" parameter is there now...
> Should we add a guidance of "when reworking code, always pass NULL as the type parameter to rte_malloc functions" somewhere in the programmers guide, to align community with this "pass NULL for type" initiative?
> 
> <snip>
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> 

Did look into Mi-Malloc https://github.com/microsoft/mimalloc
it is fast and more complete and good work with huge pages.
The way to handle tagging allocations having the library automatically handle it
based on the place allocation is called from. Having user do it is not that helpful.

Re: [PATCH] event: fix warning from useless snprintf

[Jerin Jacob]

On Thu, Apr 25, 2024 at 12:41 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 24 Apr 2024 17:12:39 +0000
> "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
>
> > > ________________________________________
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Wednesday, April 24, 2024 5:13 PM
> > > To: Van Haaren, Harry
> > > Cc: dev@dpdk.org; Richardson, Bruce; Jerin Jacob
> > > Subject: Re: [PATCH] event: fix warning from useless snprintf
> > >
> > > On Wed, 24 Apr 2024 08:45:52 +0000
> > > "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
> > >
> > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Sent: Wednesday, April 24, 2024 4:45 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: Richardson, Bruce; Stephen Hemminger; Van Haaren, Harry; Jerin Jacob
> > > > > Subject: [PATCH] event: fix warning from useless snprintf
> > > > >
> > > > > With Gcc-14, this warning is generated:
> > > > > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> > > > >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> > > > >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> > > > >       |                 ^
> > > > >
> > > > > Yet the whole printf to the buf is unnecessary. The type string argument
> > > > > has never been implemented, and should just be NULL.  Removing the
> > > > > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.
> > > >
> > > > I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> > > > seems like others are filling in "type" to be some debug-useful name/string. If it was added
> > > > in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.
> > >
> > > No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it
> >
> > I agree that YAGNI perhaps applied when designing the APIs, but the "type" parameter is there now...
> > Should we add a guidance of "when reworking code, always pass NULL as the type parameter to rte_malloc functions" somewhere in the programmers guide, to align community with this "pass NULL for type" initiative?
> >
> > <snip>
> >
> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>


Changed to event/sw:

Applied to dpdk-next-eventdev/for-main. Thanks


> >

Re: [PATCH] event: fix warning from useless snprintf

[Thomas Monjalon]

24/04/2024 21:10, Stephen Hemminger:
> On Wed, 24 Apr 2024 17:12:39 +0000
> "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > > "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:
> > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > >
> > > > > With Gcc-14, this warning is generated:
> > > > > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> > > > >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> > > > >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> > > > >       |                 ^
> > > > >
> > > > > Yet the whole printf to the buf is unnecessary. The type string argument
> > > > > has never been implemented, and should just be NULL.  Removing the
> > > > > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.  
> > > >
> > > > I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> > > > seems like others are filling in "type" to be some debug-useful name/string. If it was added
> > > > in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.  
> > > 
> > > No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it  
> > 
> > I agree that YAGNI perhaps applied when designing the APIs, but the "type" parameter is there now...
> > Should we add a guidance of "when reworking code, always pass NULL as the type parameter to rte_malloc functions" somewhere in the programmers guide, to align community with this "pass NULL for type" initiative?
> > 
> > <snip>
> > 
> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> > 
> 
> Did look into Mi-Malloc https://github.com/microsoft/mimalloc
> it is fast and more complete and good work with huge pages.
> The way to handle tagging allocations having the library automatically handle it
> based on the place allocation is called from. Having user do it is not that helpful.

But today we have rte_malloc.
And the type is used in tracing.
I think having a meaningful name from the caller is not a bad idea.

Re: [PATCH] event: fix warning from useless snprintf

[Stephen Hemminger]

On Sat, 15 Jun 2024 13:43:16 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 24/04/2024 21:10, Stephen Hemminger:
> > On Wed, 24 Apr 2024 17:12:39 +0000
> > "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:  
> > > From: Stephen Hemminger <stephen@networkplumber.org>  
> > > > "Van Haaren, Harry" <harry.van.haaren@intel.com> wrote:  
> > > > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > > >
> > > > > > With Gcc-14, this warning is generated:
> > > > > > ../drivers/event/sw/sw_evdev.c:263:3: warning: 'snprintf' will always be truncated;
> > > > > >     specified size is 12, but format string expands to at least 13 [-Wformat-truncation]
> > > > > >   263 |                 snprintf(buf, sizeof(buf), "sw%d_iq_%d_rob", dev_id, i);
> > > > > >       |                 ^
> > > > > >
> > > > > > Yet the whole printf to the buf is unnecessary. The type string argument
> > > > > > has never been implemented, and should just be NULL.  Removing the
> > > > > > unnecessary snprintf, then means IQ_ROB_NAMESIZE can be removed.    
> > > > >
> > > > > I understand that today the "type" value isn't implemented, but across the DPDK codebase it
> > > > > seems like others are filling in "type" to be some debug-useful name/string. If it was added
> > > > > in future it'd be nice to have the ROB/IQ memory identified by name, like the rest of DPDK components.    
> > > > 
> > > > No, don't bother. This is a case of https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it    
> > > 
> > > I agree that YAGNI perhaps applied when designing the APIs, but the "type" parameter is there now...
> > > Should we add a guidance of "when reworking code, always pass NULL as the type parameter to rte_malloc functions" somewhere in the programmers guide, to align community with this "pass NULL for type" initiative?
> > > 
> > > <snip>
> > > 
> > > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> > >   
> > 
> > Did look into Mi-Malloc https://github.com/microsoft/mimalloc
> > it is fast and more complete and good work with huge pages.
> > The way to handle tagging allocations having the library automatically handle it
> > based on the place allocation is called from. Having user do it is not that helpful.  
> 
> But today we have rte_malloc.
> And the type is used in tracing.
> I think having a meaningful name from the caller is not a bad idea.
> 
> 

Ok still useful for tracing, but documentation is incorrect (dump stats doesn't use it), and the
type field doesn't need to be passed as argument to heap code.