* [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header @ 2021-05-03 16:34 jerinj 2021-05-04 12:14 ` Bruce Richardson 0 siblings, 1 reply; 7+ messages in thread From: jerinj @ 2021-05-03 16:34 UTC (permalink / raw) To: Ciara Power; +Cc: dev, thomas, bruce.richardson, Jerin Jacob 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-03 16:34 [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header jerinj @ 2021-05-04 12:14 ` Bruce Richardson 2021-05-04 16:19 ` Power, Ciara 0 siblings, 1 reply; 7+ messages in thread From: Bruce Richardson @ 2021-05-04 12:14 UTC (permalink / raw) To: jerinj; +Cc: Ciara Power, dev, thomas 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-04 12:14 ` Bruce Richardson @ 2021-05-04 16:19 ` Power, Ciara 2021-05-05 8:44 ` David Marchand 2021-05-05 16:15 ` David Marchand 0 siblings, 2 replies; 7+ messages in thread From: Power, Ciara @ 2021-05-04 16:19 UTC (permalink / raw) To: Richardson, Bruce, jerinj; +Cc: dev, thomas >-----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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-04 16:19 ` Power, Ciara @ 2021-05-05 8:44 ` David Marchand 2021-05-05 10:24 ` Jerin Jacob 2021-05-05 16:15 ` David Marchand 1 sibling, 1 reply; 7+ messages in thread From: David Marchand @ 2021-05-05 8:44 UTC (permalink / raw) To: Power, Ciara, Richardson, Bruce; +Cc: jerinj, dev, thomas 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-05 8:44 ` David Marchand @ 2021-05-05 10:24 ` Jerin Jacob 2021-05-05 14:12 ` Power, Ciara 0 siblings, 1 reply; 7+ messages in thread From: Jerin Jacob @ 2021-05-05 10:24 UTC (permalink / raw) To: David Marchand; +Cc: Power, Ciara, Richardson, Bruce, jerinj, dev, thomas 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-05 10:24 ` Jerin Jacob @ 2021-05-05 14:12 ` Power, Ciara 0 siblings, 0 replies; 7+ messages in thread From: Power, Ciara @ 2021-05-05 14:12 UTC (permalink / raw) To: Jerin Jacob, David Marchand; +Cc: Richardson, Bruce, jerinj, dev, thomas >-----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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header 2021-05-04 16:19 ` Power, Ciara 2021-05-05 8:44 ` David Marchand @ 2021-05-05 16:15 ` David Marchand 1 sibling, 0 replies; 7+ messages in thread From: David Marchand @ 2021-05-05 16:15 UTC (permalink / raw) To: jerinj; +Cc: Power, Ciara, Richardson, Bruce, dev, thomas 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-05 16:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-03 16:34 [dpdk-dev] [PATCH] telemetry: remove internal symbol from public header jerinj 2021-05-04 12:14 ` Bruce Richardson 2021-05-04 16:19 ` Power, Ciara 2021-05-05 8:44 ` David Marchand 2021-05-05 10:24 ` Jerin Jacob 2021-05-05 14:12 ` Power, Ciara 2021-05-05 16:15 ` David Marchand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).