[dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[jerinj]

From: Jerin Jacob <jerinj@marvell.com>

Remove TELEMETRY_MAX_CALLBACKS symbol from public
rte_telemetry.h header file.

Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---
 lib/telemetry/rte_telemetry.h | 2 --
 lib/telemetry/telemetry.c     | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index 031db9e968..c08146e142 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -10,8 +10,6 @@
 #ifndef _RTE_TELEMETRY_H_
 #define _RTE_TELEMETRY_H_
 
-/** Maximum number of telemetry callbacks. */
-#define TELEMETRY_MAX_CALLBACKS 64
 /** Maximum length for string used in object. */
 #define RTE_TEL_MAX_STRING_LEN 64
 /** Maximum length of string. */
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 386d0080bc..c06de45531 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -27,6 +27,9 @@
 #define MAX_OUTPUT_LEN (1024 * 16)
 #define MAX_CONNECTIONS 10
 
+/** Maximum number of telemetry callbacks. */
+#define TELEMETRY_MAX_CALLBACKS 64
+
 #ifndef RTE_EXEC_ENV_WINDOWS
 static void *
 client_handler(void *socket);
-- 
2.31.1

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[Bruce Richardson]

On Mon, May 03, 2021 at 10:04:28PM +0530, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
> 
> Remove TELEMETRY_MAX_CALLBACKS symbol from public
> rte_telemetry.h header file.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[Power, Ciara]

>-----Original Message-----
>From: Richardson, Bruce <bruce.richardson@intel.com>
>Sent: Tuesday 4 May 2021 13:14
>To: jerinj@marvell.com
>Cc: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org;
>thomas@monjalon.net
>Subject: Re: [PATCH] telemetry: remove internal symbol from public header
>
>On Mon, May 03, 2021 at 10:04:28PM +0530, jerinj@marvell.com wrote:
>> From: Jerin Jacob <jerinj@marvell.com>
>>
>> Remove TELEMETRY_MAX_CALLBACKS symbol from public rte_telemetry.h
>> header file.
>>
>> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
>> ---
>Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Thanks,
Acked-by: Ciara Power <ciara.power@intel.com>

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[David Marchand]

On Tue, May 4, 2021 at 6:19 PM Power, Ciara <ciara.power@intel.com> wrote:
>
> >-----Original Message-----
> >From: Richardson, Bruce <bruce.richardson@intel.com>
> >Sent: Tuesday 4 May 2021 13:14
> >To: jerinj@marvell.com
> >Cc: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org;
> >thomas@monjalon.net
> >Subject: Re: [PATCH] telemetry: remove internal symbol from public header
> >
> >On Mon, May 03, 2021 at 10:04:28PM +0530, jerinj@marvell.com wrote:
> >> From: Jerin Jacob <jerinj@marvell.com>
> >>
> >> Remove TELEMETRY_MAX_CALLBACKS symbol from public rte_telemetry.h
> >> header file.
> >>
> >> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> >> ---
> >Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Thanks,
> Acked-by: Ciara Power <ciara.power@intel.com>

I agree this define should be hidden.

Just, what do you think of using a dynamic allocation and remove the
limitation entirely?


diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index 031db9e968..8776998b54 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -10,8 +10,6 @@
 #ifndef _RTE_TELEMETRY_H_
 #define _RTE_TELEMETRY_H_

-/** Maximum number of telemetry callbacks. */
-#define TELEMETRY_MAX_CALLBACKS 64
 /** Maximum length for string used in object. */
 #define RTE_TEL_MAX_STRING_LEN 64
 /** Maximum length of string. */
@@ -285,7 +283,7 @@ typedef void * (*handler)(void *sock_id);
  * @return
  *  -EINVAL for invalid parameters failure.
  *  @return
- *  -ENOENT if max callbacks limit has been reached.
+ *  -ENOMEM for mem allocation failure.
  */
 __rte_experimental
 int
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 68b479e0e4..6baba57ec2 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -59,7 +59,7 @@ static uint32_t logtype;
         rte_log_ptr(RTE_LOG_ ## l, logtype, "TELEMETRY: " __VA_ARGS__)

 /* list of command callbacks, with one command registered by default */
-static struct cmd_callback callbacks[TELEMETRY_MAX_CALLBACKS];
+static struct cmd_callback *callbacks;
 static int num_callbacks; /* How many commands are registered */
 /* Used when accessing or modifying list of command callbacks */
 static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
@@ -70,15 +70,21 @@ static uint16_t v2_clients;
 int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 {
+       struct cmd_callback *new_callbacks;
        int i = 0;

        if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
                        || strlen(help) >= MAX_HELP_LEN)
                return -EINVAL;
-       if (num_callbacks >= TELEMETRY_MAX_CALLBACKS)
-               return -ENOENT;

        rte_spinlock_lock(&callback_sl);
+       new_callbacks = realloc(callbacks, sizeof(callbacks[0]) *
(num_callbacks + 1));
+       if (new_callbacks == NULL) {
+               rte_spinlock_unlock(&callback_sl);
+               return -ENOMEM;
+       }
+       callbacks = new_callbacks;
+
        while (i < num_callbacks && strcmp(cmd, callbacks[i].cmd) > 0)
                i++;
        if (i != num_callbacks)


And there is a race to fix in list_commands() (which accesses the
callbacks array without taking the lock).

-- 
David Marchand

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[Jerin Jacob]

On Wed, May 5, 2021 at 2:14 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Tue, May 4, 2021 at 6:19 PM Power, Ciara <ciara.power@intel.com> wrote:
> >
> > >-----Original Message-----
> > >From: Richardson, Bruce <bruce.richardson@intel.com>
> > >Sent: Tuesday 4 May 2021 13:14
> > >To: jerinj@marvell.com
> > >Cc: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org;
> > >thomas@monjalon.net
> > >Subject: Re: [PATCH] telemetry: remove internal symbol from public header
> > >
> > >On Mon, May 03, 2021 at 10:04:28PM +0530, jerinj@marvell.com wrote:
> > >> From: Jerin Jacob <jerinj@marvell.com>
> > >>
> > >> Remove TELEMETRY_MAX_CALLBACKS symbol from public rte_telemetry.h
> > >> header file.
> > >>
> > >> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > >> ---
> > >Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > Thanks,
> > Acked-by: Ciara Power <ciara.power@intel.com>
>
> I agree this define should be hidden.
>
> Just, what do you think of using a dynamic allocation and remove the
> limitation entirely?

I think, that may be better. Probably this patch can go in rc2.

The dynamic feature can be pushed to the next release.

I will leave it to the maintainers to decide. I am fine with any scheme.

>
>
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index 031db9e968..8776998b54 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -10,8 +10,6 @@
>  #ifndef _RTE_TELEMETRY_H_
>  #define _RTE_TELEMETRY_H_
>
> -/** Maximum number of telemetry callbacks. */
> -#define TELEMETRY_MAX_CALLBACKS 64
>  /** Maximum length for string used in object. */
>  #define RTE_TEL_MAX_STRING_LEN 64
>  /** Maximum length of string. */
> @@ -285,7 +283,7 @@ typedef void * (*handler)(void *sock_id);
>   * @return
>   *  -EINVAL for invalid parameters failure.
>   *  @return
> - *  -ENOENT if max callbacks limit has been reached.
> + *  -ENOMEM for mem allocation failure.
>   */
>  __rte_experimental
>  int
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 68b479e0e4..6baba57ec2 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -59,7 +59,7 @@ static uint32_t logtype;
>          rte_log_ptr(RTE_LOG_ ## l, logtype, "TELEMETRY: " __VA_ARGS__)
>
>  /* list of command callbacks, with one command registered by default */
> -static struct cmd_callback callbacks[TELEMETRY_MAX_CALLBACKS];
> +static struct cmd_callback *callbacks;
>  static int num_callbacks; /* How many commands are registered */
>  /* Used when accessing or modifying list of command callbacks */
>  static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER;
> @@ -70,15 +70,21 @@ static uint16_t v2_clients;
>  int
>  rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
>  {
> +       struct cmd_callback *new_callbacks;
>         int i = 0;
>
>         if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
>                         || strlen(help) >= MAX_HELP_LEN)
>                 return -EINVAL;
> -       if (num_callbacks >= TELEMETRY_MAX_CALLBACKS)
> -               return -ENOENT;
>
>         rte_spinlock_lock(&callback_sl);
> +       new_callbacks = realloc(callbacks, sizeof(callbacks[0]) *
> (num_callbacks + 1));
> +       if (new_callbacks == NULL) {
> +               rte_spinlock_unlock(&callback_sl);
> +               return -ENOMEM;
> +       }
> +       callbacks = new_callbacks;
> +
>         while (i < num_callbacks && strcmp(cmd, callbacks[i].cmd) > 0)
>                 i++;
>         if (i != num_callbacks)
>
>
> And there is a race to fix in list_commands() (which accesses the
> callbacks array without taking the lock).
>
> --
> David Marchand
>

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[Power, Ciara]

>-----Original Message-----
>From: Jerin Jacob <jerinjacobk@gmail.com>
>Sent: Wednesday 5 May 2021 11:25
>To: David Marchand <david.marchand@redhat.com>
>Cc: Power, Ciara <ciara.power@intel.com>; Richardson, Bruce
><bruce.richardson@intel.com>; jerinj@marvell.com; dev@dpdk.org;
>thomas@monjalon.net
>Subject: Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from
>public header
>
>On Wed, May 5, 2021 at 2:14 PM David Marchand
><david.marchand@redhat.com> wrote:
>>
>> On Tue, May 4, 2021 at 6:19 PM Power, Ciara <ciara.power@intel.com>
>wrote:
>> >
>> > >-----Original Message-----
>> > >From: Richardson, Bruce <bruce.richardson@intel.com>
>> > >Sent: Tuesday 4 May 2021 13:14
>> > >To: jerinj@marvell.com
>> > >Cc: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org;
>> > >thomas@monjalon.net
>> > >Subject: Re: [PATCH] telemetry: remove internal symbol from public
>> > >header
>> > >
>> > >On Mon, May 03, 2021 at 10:04:28PM +0530, jerinj@marvell.com wrote:
>> > >> From: Jerin Jacob <jerinj@marvell.com>
>> > >>
>> > >> Remove TELEMETRY_MAX_CALLBACKS symbol from public
>rte_telemetry.h
>> > >> header file.
>> > >>
>> > >> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
>> > >> ---
>> > >Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>> >
>> > Thanks,
>> > Acked-by: Ciara Power <ciara.power@intel.com>
>>
>> I agree this define should be hidden.
>>
>> Just, what do you think of using a dynamic allocation and remove the
>> limitation entirely?
>
>I think, that may be better. Probably this patch can go in rc2.
>
>The dynamic feature can be pushed to the next release.
>
>I will leave it to the maintainers to decide. I am fine with any scheme.
>

+1 for the dynamic allocation idea.
I am ok with using this small fix patch for now and making the change to dynamic allocation at a later stage,
if it isn't suitable at this point in the release cycle.

>>
>>
>> diff --git a/lib/telemetry/rte_telemetry.h
>> b/lib/telemetry/rte_telemetry.h index 031db9e968..8776998b54 100644
>> --- a/lib/telemetry/rte_telemetry.h
>> +++ b/lib/telemetry/rte_telemetry.h
>> @@ -10,8 +10,6 @@
>>  #ifndef _RTE_TELEMETRY_H_
>>  #define _RTE_TELEMETRY_H_
>>
>> -/** Maximum number of telemetry callbacks. */ -#define
>> TELEMETRY_MAX_CALLBACKS 64
>>  /** Maximum length for string used in object. */  #define
>> RTE_TEL_MAX_STRING_LEN 64
>>  /** Maximum length of string. */
>> @@ -285,7 +283,7 @@ typedef void * (*handler)(void *sock_id);
>>   * @return
>>   *  -EINVAL for invalid parameters failure.
>>   *  @return
>> - *  -ENOENT if max callbacks limit has been reached.
>> + *  -ENOMEM for mem allocation failure.
>>   */
>>  __rte_experimental
>>  int
>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>> index 68b479e0e4..6baba57ec2 100644
>> --- a/lib/telemetry/telemetry.c
>> +++ b/lib/telemetry/telemetry.c
>> @@ -59,7 +59,7 @@ static uint32_t logtype;
>>          rte_log_ptr(RTE_LOG_ ## l, logtype, "TELEMETRY: "
>> __VA_ARGS__)
>>
>>  /* list of command callbacks, with one command registered by default
>> */ -static struct cmd_callback callbacks[TELEMETRY_MAX_CALLBACKS];
>> +static struct cmd_callback *callbacks;
>>  static int num_callbacks; /* How many commands are registered */
>>  /* Used when accessing or modifying list of command callbacks */
>> static rte_spinlock_t callback_sl = RTE_SPINLOCK_INITIALIZER; @@
>> -70,15 +70,21 @@ static uint16_t v2_clients;  int
>> rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const
>> char *help)  {
>> +       struct cmd_callback *new_callbacks;
>>         int i = 0;
>>
>>         if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
>>                         || strlen(help) >= MAX_HELP_LEN)
>>                 return -EINVAL;
>> -       if (num_callbacks >= TELEMETRY_MAX_CALLBACKS)
>> -               return -ENOENT;
>>
>>         rte_spinlock_lock(&callback_sl);
>> +       new_callbacks = realloc(callbacks, sizeof(callbacks[0]) *
>> (num_callbacks + 1));
>> +       if (new_callbacks == NULL) {
>> +               rte_spinlock_unlock(&callback_sl);
>> +               return -ENOMEM;
>> +       }
>> +       callbacks = new_callbacks;
>> +
>>         while (i < num_callbacks && strcmp(cmd, callbacks[i].cmd) > 0)
>>                 i++;
>>         if (i != num_callbacks)
>>
>>
>> And there is a race to fix in list_commands() (which accesses the
>> callbacks array without taking the lock).
>>
>> --
>> David Marchand
>>

Good catch, I can send a fix patch for this list commands function. Thanks David.

- Ciara 

Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header

[David Marchand]

On Tue, May 4, 2021 at 6:19 PM Power, Ciara <ciara.power@intel.com> wrote:
> >> Remove TELEMETRY_MAX_CALLBACKS symbol from public rte_telemetry.h
> >> header file.
> >>
> >> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> >Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>

Applied, thanks.

I'll post my suggestion as a patch aimed at 21.08.


-- 
David Marchand