DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
@ 2020-10-05 20:02 Timothy McDaniel
  2020-10-06  8:45 ` David Marchand
  2020-10-08  7:33 ` [dpdk-dev] " David Marchand
  0 siblings, 2 replies; 9+ messages in thread
From: Timothy McDaniel @ 2020-10-05 20:02 UTC (permalink / raw)
  To: Jerin Jacob, Sunil Kumar Kori
  Cc: dev, erik.g.carrillo, gage.eads, harry.van.haaren

Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
Fixes "CTF field is too long" error when running with trace enabled.

Signed-off-by: Timothy McDaniel <timothy.mcdaniel@intel.com>
---
 lib/librte_eal/common/eal_trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
index 92c5951..438c2b7 100644
--- a/lib/librte_eal/common/eal_trace.h
+++ b/lib/librte_eal/common/eal_trace.h
@@ -24,7 +24,7 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_CTF_FIELD_SIZE 384
+#define TRACE_CTF_FIELD_SIZE 448
 #define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
-- 
2.6.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-05 20:02 [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size Timothy McDaniel
@ 2020-10-06  8:45 ` David Marchand
  2020-10-06  9:22   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
  2020-10-08  7:33 ` [dpdk-dev] " David Marchand
  1 sibling, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-10-06  8:45 UTC (permalink / raw)
  To: Timothy McDaniel
  Cc: Jerin Jacob, Sunil Kumar Kori, dev, Erik Gabriel Carrillo,
	Gage Eads, Van Haaren Harry

On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
<timothy.mcdaniel@intel.com> wrote:
>
> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.

Repeating the same sentence in the title and the commitlog does not
give much info.

Plus, what is this "recommendation"?


> Fixes "CTF field is too long" error when running with trace enabled.

Ok, you hit this limit, but it would help to get some context here.
Looking at this patch in the future, we won't know why it was necessary.


>
> Signed-off-by: Timothy McDaniel <timothy.mcdaniel@intel.com>
> ---
>  lib/librte_eal/common/eal_trace.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
> index 92c5951..438c2b7 100644
> --- a/lib/librte_eal/common/eal_trace.h
> +++ b/lib/librte_eal/common/eal_trace.h
> @@ -24,7 +24,7 @@
>
>  #define TRACE_PREFIX_LEN 12
>  #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
> -#define TRACE_CTF_FIELD_SIZE 384
> +#define TRACE_CTF_FIELD_SIZE 448
>  #define TRACE_POINT_NAME_SIZE 64
>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>  #define TRACE_MAX_ARGS 32
> --
> 2.6.4
>


-- 
David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-06  8:45 ` David Marchand
@ 2020-10-06  9:22   ` Sunil Kumar Kori
  2020-10-06  9:39     ` David Marchand
  0 siblings, 1 reply; 9+ messages in thread
From: Sunil Kumar Kori @ 2020-10-06  9:22 UTC (permalink / raw)
  To: David Marchand, Timothy McDaniel
  Cc: Jerin Jacob Kollanukkaran, dev, Erik Gabriel Carrillo, Gage Eads,
	Van Haaren Harry



Regards
Sunil Kumar Kori

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Tuesday, October 6, 2020 2:15 PM
>To: Timothy McDaniel <timothy.mcdaniel@intel.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>; dev <dev@dpdk.org>; Erik Gabriel Carrillo
><erik.g.carrillo@intel.com>; Gage Eads <gage.eads@intel.com>; Van Haaren
>Harry <harry.van.haaren@intel.com>
>Subject: [EXT] Re: [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to
>recommended size
>
>External Email
>
>----------------------------------------------------------------------
>On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
><timothy.mcdaniel@intel.com> wrote:
>>
>> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
>
>Repeating the same sentence in the title and the commitlog does not give
>much info.
>
>Plus, what is this "recommendation"?
When analyzed this issue, only one more byte was needed to fix this issue but in future similar issue can occur again.
So increasing this value by 64 bytes which actually equals to a cache line. That’s why we have suggested this size.

>
>
>> Fixes "CTF field is too long" error when running with trace enabled.
>
>Ok, you hit this limit, but it would help to get some context here.
>Looking at this patch in the future, we won't know why it was necessary.
>
>
>>
>> Signed-off-by: Timothy McDaniel <timothy.mcdaniel@intel.com>
>> ---
>>  lib/librte_eal/common/eal_trace.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_eal/common/eal_trace.h
>> b/lib/librte_eal/common/eal_trace.h
>> index 92c5951..438c2b7 100644
>> --- a/lib/librte_eal/common/eal_trace.h
>> +++ b/lib/librte_eal/common/eal_trace.h
>> @@ -24,7 +24,7 @@
>>
>>  #define TRACE_PREFIX_LEN 12
>>  #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") +
>> TRACE_PREFIX_LEN) -#define TRACE_CTF_FIELD_SIZE 384
>> +#define TRACE_CTF_FIELD_SIZE 448
>>  #define TRACE_POINT_NAME_SIZE 64
>>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>>  #define TRACE_MAX_ARGS 32
>> --
>> 2.6.4
>>
>
>
>--
>David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-06  9:22   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
@ 2020-10-06  9:39     ` David Marchand
  2020-10-06  9:58       ` Jerin Jacob
  0 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-10-06  9:39 UTC (permalink / raw)
  To: Sunil Kumar Kori, Timothy McDaniel
  Cc: Jerin Jacob Kollanukkaran, dev, Erik Gabriel Carrillo, Gage Eads,
	Van Haaren Harry

On Tue, Oct 6, 2020 at 11:22 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> >On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
> ><timothy.mcdaniel@intel.com> wrote:
> >>
> >> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
> >
> >Repeating the same sentence in the title and the commitlog does not give
> >much info.
> >
> >Plus, what is this "recommendation"?
> When analyzed this issue, only one more byte was needed to fix this issue but in future similar issue can occur again.
> So increasing this value by 64 bytes which actually equals to a cache line. That’s why we have suggested this size.

384 is aligned to both 64 and 128 bytes cache lines.
448 is only aligned to 64 bytes.

Should we care about 128 bytes cache lines systems?

>
> >
> >
> >> Fixes "CTF field is too long" error when running with trace enabled.
> >
> >Ok, you hit this limit, but it would help to get some context here.
> >Looking at this patch in the future, we won't know why it was necessary.

How about following commitlog:

"""
trace: increase trace point buffer size

The current buffer size is not big enough to accomodate traces for new
additions in the eventdev subsystem.
Increase this buffer size by XXX for reason YYY.
"""


-- 
David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-06  9:39     ` David Marchand
@ 2020-10-06  9:58       ` Jerin Jacob
  2020-10-07  9:04         ` David Marchand
  0 siblings, 1 reply; 9+ messages in thread
From: Jerin Jacob @ 2020-10-06  9:58 UTC (permalink / raw)
  To: David Marchand
  Cc: Sunil Kumar Kori, Timothy McDaniel, Jerin Jacob Kollanukkaran,
	dev, Erik Gabriel Carrillo, Gage Eads, Van Haaren Harry

On Tue, Oct 6, 2020 at 3:10 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Tue, Oct 6, 2020 at 11:22 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> > >On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
> > ><timothy.mcdaniel@intel.com> wrote:
> > >>
> > >> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
> > >
> > >Repeating the same sentence in the title and the commitlog does not give
> > >much info.
> > >
> > >Plus, what is this "recommendation"?
> > When analyzed this issue, only one more byte was needed to fix this issue but in future similar issue can occur again.
> > So increasing this value by 64 bytes which actually equals to a cache line. That’s why we have suggested this size.
>
> 384 is aligned to both 64 and 128 bytes cache lines.
> 448 is only aligned to 64 bytes.
>
> Should we care about 128 bytes cache lines systems?

it is on a slow path. 448 is OK.

>
> >
> > >
> > >
> > >> Fixes "CTF field is too long" error when running with trace enabled.
> > >
> > >Ok, you hit this limit, but it would help to get some context here.
> > >Looking at this patch in the future, we won't know why it was necessary.
>
> How about following commitlog:
>
> """
> trace: increase trace point buffer size
>
> The current buffer size is not big enough to accomodate traces for new
> additions in the eventdev subsystem.
> Increase this buffer size by XXX for reason YYY.
> """

Looks good to me.

>
>
> --
> David Marchand
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-06  9:58       ` Jerin Jacob
@ 2020-10-07  9:04         ` David Marchand
  2020-10-07 10:07           ` Sunil Kumar Kori
  0 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-10-07  9:04 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Sunil Kumar Kori, Timothy McDaniel, Jerin Jacob Kollanukkaran,
	dev, Erik Gabriel Carrillo, Gage Eads, Van Haaren Harry

On Tue, Oct 6, 2020 at 11:58 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Tue, Oct 6, 2020 at 3:10 PM David Marchand <david.marchand@redhat.com> wrote:
> >
> > On Tue, Oct 6, 2020 at 11:22 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> > > >On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
> > > ><timothy.mcdaniel@intel.com> wrote:
> > > >>
> > > >> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
> > > >
> > > >Repeating the same sentence in the title and the commitlog does not give
> > > >much info.
> > > >
> > > >Plus, what is this "recommendation"?
> > > When analyzed this issue, only one more byte was needed to fix this issue but in future similar issue can occur again.
> > > So increasing this value by 64 bytes which actually equals to a cache line. That’s why we have suggested this size.
> >
> > 384 is aligned to both 64 and 128 bytes cache lines.
> > 448 is only aligned to 64 bytes.
> >
> > Should we care about 128 bytes cache lines systems?
>
> it is on a slow path. 448 is OK.

Ah yes, this is for the ctf description.
Could it be changed to rely on dynamic allocations and we simply
remove this limit?


>
> >
> > >
> > > >
> > > >
> > > >> Fixes "CTF field is too long" error when running with trace enabled.
> > > >
> > > >Ok, you hit this limit, but it would help to get some context here.
> > > >Looking at this patch in the future, we won't know why it was necessary.
> >
> > How about following commitlog:
> >
> > """
> > trace: increase trace point buffer size
> >
> > The current buffer size is not big enough to accomodate traces for new
> > additions in the eventdev subsystem.
> > Increase this buffer size by XXX for reason YYY.
> > """
>
> Looks good to me.

Well in this case, there is no actual reason.
The increased value is deemed "enough for now", unless we change this
to dynamic allocations.



-- 
David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-07  9:04         ` David Marchand
@ 2020-10-07 10:07           ` Sunil Kumar Kori
  2020-10-07 12:34             ` David Marchand
  0 siblings, 1 reply; 9+ messages in thread
From: Sunil Kumar Kori @ 2020-10-07 10:07 UTC (permalink / raw)
  To: David Marchand, Jerin Jacob
  Cc: Timothy McDaniel, Jerin Jacob Kollanukkaran, dev,
	Erik Gabriel Carrillo, Gage Eads, Van Haaren Harry



Regards
Sunil Kumar Kori

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Wednesday, October 7, 2020 2:34 PM
>To: Jerin Jacob <jerinjacobk@gmail.com>
>Cc: Sunil Kumar Kori <skori@marvell.com>; Timothy McDaniel
><timothy.mcdaniel@intel.com>; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; dev <dev@dpdk.org>; Erik Gabriel Carrillo
><erik.g.carrillo@intel.com>; Gage Eads <gage.eads@intel.com>; Van Haaren
>Harry <harry.van.haaren@intel.com>
>Subject: Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to
>recommended size
>
>On Tue, Oct 6, 2020 at 11:58 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>
>> On Tue, Oct 6, 2020 at 3:10 PM David Marchand
><david.marchand@redhat.com> wrote:
>> >
>> > On Tue, Oct 6, 2020 at 11:22 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>> > > >On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
>> > > ><timothy.mcdaniel@intel.com> wrote:
>> > > >>
>> > > >> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
>> > > >
>> > > >Repeating the same sentence in the title and the commitlog does
>> > > >not give much info.
>> > > >
>> > > >Plus, what is this "recommendation"?
>> > > When analyzed this issue, only one more byte was needed to fix this
>issue but in future similar issue can occur again.
>> > > So increasing this value by 64 bytes which actually equals to a cache line.
>That’s why we have suggested this size.
>> >
>> > 384 is aligned to both 64 and 128 bytes cache lines.
>> > 448 is only aligned to 64 bytes.
>> >
>> > Should we care about 128 bytes cache lines systems?
>>
>> it is on a slow path. 448 is OK.
>
>Ah yes, this is for the ctf description.
>Could it be changed to rely on dynamic allocations and we simply remove this
>limit?
Changing it to dynamic allocation is difficult because if we do this then every time memory is to reallocated.
So IMO, It is okay to keep it static with enough size.
>
>
>>
>> >
>> > >
>> > > >
>> > > >
>> > > >> Fixes "CTF field is too long" error when running with trace enabled.
>> > > >
>> > > >Ok, you hit this limit, but it would help to get some context here.
>> > > >Looking at this patch in the future, we won't know why it was
>necessary.
>> >
>> > How about following commitlog:
>> >
>> > """
>> > trace: increase trace point buffer size
>> >
>> > The current buffer size is not big enough to accomodate traces for
>> > new additions in the eventdev subsystem.
>> > Increase this buffer size by XXX for reason YYY.
>> > """
>>
>> Looks good to me.
>
>Well in this case, there is no actual reason.
>The increased value is deemed "enough for now", unless we change this to
>dynamic allocations.
>
>
>
>--
>David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [EXT] Re: [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-07 10:07           ` Sunil Kumar Kori
@ 2020-10-07 12:34             ` David Marchand
  0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2020-10-07 12:34 UTC (permalink / raw)
  To: Sunil Kumar Kori, Timothy McDaniel
  Cc: Jerin Jacob, dev, Erik Gabriel Carrillo, Gage Eads, Van Haaren Harry

On Wed, Oct 7, 2020 at 12:07 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>> it is on a slow path. 448 is OK.
> >
> >Ah yes, this is for the ctf description.
> >Could it be changed to rely on dynamic allocations and we simply remove this
> >limit?
> Changing it to dynamic allocation is difficult because if we do this then every time memory is to reallocated.
> So IMO, It is okay to keep it static with enough size.

Ok.
I will take this as a ack from both Jerin and you and push with the
suggested commitlog.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size
  2020-10-05 20:02 [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size Timothy McDaniel
  2020-10-06  8:45 ` David Marchand
@ 2020-10-08  7:33 ` David Marchand
  1 sibling, 0 replies; 9+ messages in thread
From: David Marchand @ 2020-10-08  7:33 UTC (permalink / raw)
  To: Timothy McDaniel
  Cc: Jerin Jacob, Sunil Kumar Kori, dev, Erik Gabriel Carrillo,
	Gage Eads, Van Haaren Harry

On Mon, Oct 5, 2020 at 10:16 PM Timothy McDaniel
<timothy.mcdaniel@intel.com> wrote:
>
> Increase TRACE_CTF_FIELD_SIZE to 448, the recommended size.
> Fixes "CTF field is too long" error when running with trace enabled.
>
> Signed-off-by: Timothy McDaniel <timothy.mcdaniel@intel.com>

Applied with added acks from Jerin and Sunil, thanks.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-08  7:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 20:02 [dpdk-dev] [PATCH 1/1] eal: increase TRACE CTF SIZE to recommended size Timothy McDaniel
2020-10-06  8:45 ` David Marchand
2020-10-06  9:22   ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-10-06  9:39     ` David Marchand
2020-10-06  9:58       ` Jerin Jacob
2020-10-07  9:04         ` David Marchand
2020-10-07 10:07           ` Sunil Kumar Kori
2020-10-07 12:34             ` David Marchand
2020-10-08  7:33 ` [dpdk-dev] " 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).