DPDK patches and discussions
 help / color / mirror / Atom feed
* Updating examples which use coremask parameters
@ 2023-11-02 14:56 Bruce Richardson
  2023-11-02 16:28 ` Thomas Monjalon
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Bruce Richardson @ 2023-11-02 14:56 UTC (permalink / raw)
  To: dev, techboard; +Cc: Euan Bourke

Hi all,

looking to start a discussion and get some input here.

There are a number of our examples in DPDK which still track core usage via
a 64-bit bitmask, and, as such, cannot run on cores between 64 and
RTE_MAX_LCORE. Two examples I have recently come across with this issue are
"eventdev_pipeline" and "qos_sched", but I am sure there are others. The
former is a good example (or bad example depending on your viewpoint) of
this as it takes multiple coremask parameters - for RX cores, for TX cores,
for worker cores and optionally for scheduler cores.

Now, the simple solution to this is to just expand the 64-bit bitmask to
128 bit or more, but I think that is just making things harder for the
user, since dealing with long bitmasks is very awkward and unwieldy. Better
instead to convert all examples using coremasks to using core lists
instead.

First step should be to take our EAL corelist processing code and refactor
it into a function that can be made public, so that it can be used by all
apps for parsing core lists. Simple enough!

The next part I'm looking for input on is - how do we switch the apps from
coremasks to core lists? Some options:

1. Add in new commandline parameters for each app to work with core lists.
  This is what we did in the past with EAL, by adding -l as a replacement
  for -c. The advantage is that we maintain backward compatibility, but the
  downside is that it becomes hard to find new suitable letter options for
  the core lists. Taking eventdev_pipeline again, we would need "new"
  options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
  versions of these would be a simple alternative, but "-W" is already used
  as an app parameter so we can't do that.

2. Just break backward compatibility and switch the apps to taking
  core lists instead of masks. Advantage is that it gives us the cleanest
  solution, but the downside is that and testing done using these examples,
  or any users who may have run them in the past, get different behaviour.

3. An interesting further alternative is to allow apps to take both
  coremasks and corelists and use heuristics to determine which is which.
  For example, anything starting with "0x" is a mask, anything containing
  "-" or "," is a list. There would be ambiguous values such as e.g. 2,
  which could be either, but we can always find ways to disambiguate these,
  e.g. allow trailing commas in lists, so that "0x2" is the coremask, and "2,"
  is the corelist. [Could be other alternatives]. This largely keeps backward
  compatibility and also allows use of corelists.

4. something else??

Thoughts and feedback, please? We'd like to upstream some fixes for the
examples in 2024 and would rather get agreement on the approach now than
head down a wrong approach. Personally, I'd rather avoid #1, and #3 is
neat, but perhaps being overly smart/complicated?

Regards,
/Bruce

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

* Re: Updating examples which use coremask parameters
  2023-11-02 14:56 Updating examples which use coremask parameters Bruce Richardson
@ 2023-11-02 16:28 ` Thomas Monjalon
  2023-11-02 16:58   ` Bruce Richardson
  2023-11-02 17:05 ` Morten Brørup
  2023-11-03 10:11 ` Konstantin Ananyev
  2 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2023-11-02 16:28 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, techboard, Euan Bourke

02/11/2023 15:56, Bruce Richardson:
> Hi all,
> 
> looking to start a discussion and get some input here.
> 
> There are a number of our examples in DPDK which still track core usage via
> a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> former is a good example (or bad example depending on your viewpoint) of
> this as it takes multiple coremask parameters - for RX cores, for TX cores,
> for worker cores and optionally for scheduler cores.
> 
> Now, the simple solution to this is to just expand the 64-bit bitmask to
> 128 bit or more, but I think that is just making things harder for the
> user, since dealing with long bitmasks is very awkward and unwieldy. Better
> instead to convert all examples using coremasks to using core lists
> instead.
> 
> First step should be to take our EAL corelist processing code and refactor
> it into a function that can be made public, so that it can be used by all
> apps for parsing core lists. Simple enough!

OK to add some command line parsing helpers.
It should probably be the start of a new library for command line.

> The next part I'm looking for input on is - how do we switch the apps from
> coremasks to core lists? Some options:
> 
> 1. Add in new commandline parameters for each app to work with core lists.
>   This is what we did in the past with EAL, by adding -l as a replacement
>   for -c. The advantage is that we maintain backward compatibility, but the
>   downside is that it becomes hard to find new suitable letter options for
>   the core lists. Taking eventdev_pipeline again, we would need "new"
>   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
>   versions of these would be a simple alternative, but "-W" is already used
>   as an app parameter so we can't do that.
> 
> 2. Just break backward compatibility and switch the apps to taking
>   core lists instead of masks. Advantage is that it gives us the cleanest
>   solution, but the downside is that and testing done using these examples,
>   or any users who may have run them in the past, get different behaviour.

We don't need to offer backward compatibility in examples.
So this option is acceptable.

> 3. An interesting further alternative is to allow apps to take both
>   coremasks and corelists and use heuristics to determine which is which.
>   For example, anything starting with "0x" is a mask, anything containing
>   "-" or "," is a list. There would be ambiguous values such as e.g. 2,
>   which could be either, but we can always find ways to disambiguate these,
>   e.g. allow trailing commas in lists, so that "0x2" is the coremask, and "2,"
>   is the corelist. [Could be other alternatives]. This largely keeps backward
>   compatibility and also allows use of corelists.

The option 3 can be interesting as well.

> 4. something else??
> 
> Thoughts and feedback, please? We'd like to upstream some fixes for the
> examples in 2024 and would rather get agreement on the approach now than
> head down a wrong approach. Personally, I'd rather avoid #1, and #3 is
> neat, but perhaps being overly smart/complicated?
> 
> Regards,
> /Bruce



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

* Re: Updating examples which use coremask parameters
  2023-11-02 16:28 ` Thomas Monjalon
@ 2023-11-02 16:58   ` Bruce Richardson
  2023-11-06 19:19     ` Stephen Hemminger
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2023-11-02 16:58 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, techboard, Euan Bourke

On Thu, Nov 02, 2023 at 05:28:42PM +0100, Thomas Monjalon wrote:
> 02/11/2023 15:56, Bruce Richardson:
> > Hi all,
> > 
> > looking to start a discussion and get some input here.
> > 
> > There are a number of our examples in DPDK which still track core usage via
> > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> > "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> > former is a good example (or bad example depending on your viewpoint) of
> > this as it takes multiple coremask parameters - for RX cores, for TX cores,
> > for worker cores and optionally for scheduler cores.
> > 
> > Now, the simple solution to this is to just expand the 64-bit bitmask to
> > 128 bit or more, but I think that is just making things harder for the
> > user, since dealing with long bitmasks is very awkward and unwieldy. Better
> > instead to convert all examples using coremasks to using core lists
> > instead.
> > 
> > First step should be to take our EAL corelist processing code and refactor
> > it into a function that can be made public, so that it can be used by all
> > apps for parsing core lists. Simple enough!
> 
> OK to add some command line parsing helpers.
> It should probably be the start of a new library for command line.
> 

Funnily enough, separate to this I had already been working on an
"rte_args" library to have some functions for working on argc/argv
parameters. I'm planning on pushing out an RFC for 24.03 fairly shortly.

However, pulling in functions for arg parsing is a different set of
functionality to what I had in mind, so we may yet get two libraries out of
this. [Merging may be tricky due to issues around circular dependencies
with EAL. My arg management library is designed to "sit on top of" EAL,
while any lib offering e.g. coremask, corelist parsing functions would need
to "sit beneath" EAL, so EAL can re-use it's functions].

Let's postpone the details of both these to when we get some RFCs out
though.


> > The next part I'm looking for input on is - how do we switch the apps from
> > coremasks to core lists? Some options:
> > 
> > 1. Add in new commandline parameters for each app to work with core lists.
> >   This is what we did in the past with EAL, by adding -l as a replacement
> >   for -c. The advantage is that we maintain backward compatibility, but the
> >   downside is that it becomes hard to find new suitable letter options for
> >   the core lists. Taking eventdev_pipeline again, we would need "new"
> >   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
> >   versions of these would be a simple alternative, but "-W" is already used
> >   as an app parameter so we can't do that.
> > 
> > 2. Just break backward compatibility and switch the apps to taking
> >   core lists instead of masks. Advantage is that it gives us the cleanest
> >   solution, but the downside is that and testing done using these examples,
> >   or any users who may have run them in the past, get different behaviour.
> 
> We don't need to offer backward compatibility in examples.
> So this option is acceptable.
> 

Glad to hear it. Makes life simpler.

> > 3. An interesting further alternative is to allow apps to take both
> > coremasks and corelists and use heuristics to determine which is which.
> > For example, anything starting with "0x" is a mask, anything containing
> > "-" or "," is a list. There would be ambiguous values such as e.g. 2,
> > which could be either, but we can always find ways to disambiguate
> > these, e.g. allow trailing commas in lists, so that "0x2" is the
> > coremask, and "2," is the corelist. [Could be other alternatives]. This
> > largely keeps backward compatibility and also allows use of corelists.
> 
> The option 3 can be interesting as well.
>
Yep. If we start offering a library of arg-parsing functions, one of those
could be a function using heuristics to identify core-mask, core-list or
ambiguous values. Then each app can decide what to do in the latter case.
Since we don't care about backward compatibility, the examples can just parse
ambiguous values as core-list. User could then still use coremasks by
prefixing them with "0x".

/Bruce

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

* RE: Updating examples which use coremask parameters
  2023-11-02 14:56 Updating examples which use coremask parameters Bruce Richardson
  2023-11-02 16:28 ` Thomas Monjalon
@ 2023-11-02 17:05 ` Morten Brørup
  2023-11-02 17:15   ` Bruce Richardson
  2023-11-03 10:11 ` Konstantin Ananyev
  2 siblings, 1 reply; 10+ messages in thread
From: Morten Brørup @ 2023-11-02 17:05 UTC (permalink / raw)
  To: Bruce Richardson, dev, techboard; +Cc: Euan Bourke

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 2 November 2023 15.57
> 
> Hi all,
> 
> looking to start a discussion and get some input here.
> 
> There are a number of our examples in DPDK which still track core usage
> via
> a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> RTE_MAX_LCORE. Two examples I have recently come across with this issue
> are
> "eventdev_pipeline" and "qos_sched", but I am sure there are others.
> The
> former is a good example (or bad example depending on your viewpoint)
> of
> this as it takes multiple coremask parameters - for RX cores, for TX
> cores,
> for worker cores and optionally for scheduler cores.
> 
> Now, the simple solution to this is to just expand the 64-bit bitmask
> to
> 128 bit or more, but I think that is just making things harder for the
> user, since dealing with long bitmasks is very awkward and unwieldy.
> Better
> instead to convert all examples using coremasks to using core lists
> instead.
> 
> First step should be to take our EAL corelist processing code and
> refactor
> it into a function that can be made public, so that it can be used by
> all
> apps for parsing core lists. Simple enough!

If not already there, consider adding support for open-ended lists, e.g. "2-" means from 2 to the rest of the available cores.

> 
> The next part I'm looking for input on is - how do we switch the apps
> from
> coremasks to core lists? Some options:
> 
> 1. Add in new commandline parameters for each app to work with core
> lists.
>   This is what we did in the past with EAL, by adding -l as a
> replacement
>   for -c. The advantage is that we maintain backward compatibility, but
> the
>   downside is that it becomes hard to find new suitable letter options
> for
>   the core lists. Taking eventdev_pipeline again, we would need "new"
>   options for "-r", "-t", "-w" and "-s" parameters. Using the
> capitalized
>   versions of these would be a simple alternative, but "-W" is already
> used
>   as an app parameter so we can't do that.
> 
> 2. Just break backward compatibility and switch the apps to taking
>   core lists instead of masks. Advantage is that it gives us the
> cleanest
>   solution, but the downside is that and testing done using these
> examples,
>   or any users who may have run them in the past, get different
> behaviour.

I'm in favor of 2.

Coremasks are obsolete. Examples should not use them as parameters or internally.

We could emit an informational log message if a given corelist parameter could be a coremask (i.e. if it also conforms to coremask formatting).
When enough time has passed since introducing this change, this check (and associated log message) could be removed.

> 
> 3. An interesting further alternative is to allow apps to take both
>   coremasks and corelists and use heuristics to determine which is
> which.
>   For example, anything starting with "0x" is a mask, anything
> containing
>   "-" or "," is a list. There would be ambiguous values such as e.g. 2,
>   which could be either, but we can always find ways to disambiguate
> these,
>   e.g. allow trailing commas in lists, so that "0x2" is the coremask,
> and "2,"
>   is the corelist. [Could be other alternatives]. This largely keeps
> backward
>   compatibility and also allows use of corelists.
> 
> 4. something else??
> 
> Thoughts and feedback, please? We'd like to upstream some fixes for the
> examples in 2024 and would rather get agreement on the approach now
> than
> head down a wrong approach. Personally, I'd rather avoid #1, and #3 is
> neat, but perhaps being overly smart/complicated?
> 
> Regards,
> /Bruce

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

* Re: Updating examples which use coremask parameters
  2023-11-02 17:05 ` Morten Brørup
@ 2023-11-02 17:15   ` Bruce Richardson
  0 siblings, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2023-11-02 17:15 UTC (permalink / raw)
  To: Morten Brørup; +Cc: dev, techboard, Euan Bourke

On Thu, Nov 02, 2023 at 06:05:59PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 2 November 2023 15.57
> > 
> > Hi all,
> > 
> > looking to start a discussion and get some input here.
> > 
> > There are a number of our examples in DPDK which still track core usage
> > via
> > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > RTE_MAX_LCORE. Two examples I have recently come across with this issue
> > are
> > "eventdev_pipeline" and "qos_sched", but I am sure there are others.
> > The
> > former is a good example (or bad example depending on your viewpoint)
> > of
> > this as it takes multiple coremask parameters - for RX cores, for TX
> > cores,
> > for worker cores and optionally for scheduler cores.
> > 
> > Now, the simple solution to this is to just expand the 64-bit bitmask
> > to
> > 128 bit or more, but I think that is just making things harder for the
> > user, since dealing with long bitmasks is very awkward and unwieldy.
> > Better
> > instead to convert all examples using coremasks to using core lists
> > instead.
> > 
> > First step should be to take our EAL corelist processing code and
> > refactor
> > it into a function that can be made public, so that it can be used by
> > all
> > apps for parsing core lists. Simple enough!
> 
> If not already there, consider adding support for open-ended lists, e.g. "2-" means from 2 to the rest of the available cores.
> 

Interesting proposal. It's not there yet, we'll have to look into it.

> > 
> > The next part I'm looking for input on is - how do we switch the apps
> > from
> > coremasks to core lists? Some options:
> > 
> > 1. Add in new commandline parameters for each app to work with core
> > lists.
> >   This is what we did in the past with EAL, by adding -l as a
> > replacement
> >   for -c. The advantage is that we maintain backward compatibility, but
> > the
> >   downside is that it becomes hard to find new suitable letter options
> > for
> >   the core lists. Taking eventdev_pipeline again, we would need "new"
> >   options for "-r", "-t", "-w" and "-s" parameters. Using the
> > capitalized
> >   versions of these would be a simple alternative, but "-W" is already
> > used
> >   as an app parameter so we can't do that.
> > 
> > 2. Just break backward compatibility and switch the apps to taking
> >   core lists instead of masks. Advantage is that it gives us the
> > cleanest
> >   solution, but the downside is that and testing done using these
> > examples,
> >   or any users who may have run them in the past, get different
> > behaviour.
> 
> I'm in favor of 2.
> 
> Coremasks are obsolete. Examples should not use them as parameters or internally.
> 
> We could emit an informational log message if a given corelist parameter could be a coremask (i.e. if it also conforms to coremask formatting).
> When enough time has passed since introducing this change, this check (and associated log message) could be removed.
> 

Thanks for the feedback.

/Bruce

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

* RE: Updating examples which use coremask parameters
  2023-11-02 14:56 Updating examples which use coremask parameters Bruce Richardson
  2023-11-02 16:28 ` Thomas Monjalon
  2023-11-02 17:05 ` Morten Brørup
@ 2023-11-03 10:11 ` Konstantin Ananyev
  2023-11-03 10:16   ` Bruce Richardson
  2 siblings, 1 reply; 10+ messages in thread
From: Konstantin Ananyev @ 2023-11-03 10:11 UTC (permalink / raw)
  To: Bruce Richardson, dev, techboard; +Cc: Euan Bourke


> Hi all,
> 
> looking to start a discussion and get some input here.
> 
> There are a number of our examples in DPDK which still track core usage via
> a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> former is a good example (or bad example depending on your viewpoint) of
> this as it takes multiple coremask parameters - for RX cores, for TX cores,
> for worker cores and optionally for scheduler cores.
> 
> Now, the simple solution to this is to just expand the 64-bit bitmask to
> 128 bit or more, but I think that is just making things harder for the
> user, since dealing with long bitmasks is very awkward and unwieldy. Better
> instead to convert all examples using coremasks to using core lists
> instead.
> 
> First step should be to take our EAL corelist processing code and refactor
> it into a function that can be made public, so that it can be used by all
> apps for parsing core lists. Simple enough!
> 
> The next part I'm looking for input on is - how do we switch the apps from
> coremasks to core lists? Some options:
> 
> 1. Add in new commandline parameters for each app to work with core lists.
>   This is what we did in the past with EAL, by adding -l as a replacement
>   for -c. The advantage is that we maintain backward compatibility, but the
>   downside is that it becomes hard to find new suitable letter options for
>   the core lists. Taking eventdev_pipeline again, we would need "new"
>   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
>   versions of these would be a simple alternative, but "-W" is already used
>   as an app parameter so we can't do that.
> 
> 2. Just break backward compatibility and switch the apps to taking
>   core lists instead of masks. Advantage is that it gives us the cleanest
>   solution, but the downside is that and testing done using these examples,
>   or any users who may have run them in the past, get different behaviour.


As it is for examples, I also don't see any issue with converting to core-list.
Said that, I suppose we still want to keep EAL '-c' (coremask) parameter, right?
If so, then it might be plausible to consider making the code that handles it
to work with really-long ones (up to 1K, or whatever is our current CPU_SET limit).
Then if we'll have such function as a public one, users can still probably continue
to use core-mask/core-list at their best convenience.  

> 
> 3. An interesting further alternative is to allow apps to take both
>   coremasks and corelists and use heuristics to determine which is which.
>   For example, anything starting with "0x" is a mask, anything containing
>   "-" or "," is a list. There would be ambiguous values such as e.g. 2,
>   which could be either, but we can always find ways to disambiguate these,
>   e.g. allow trailing commas in lists, so that "0x2" is the coremask, and "2,"
>   is the corelist. [Could be other alternatives]. This largely keeps backward
>   compatibility and also allows use of corelists.
> 
> 4. something else??
> 
> Thoughts and feedback, please? We'd like to upstream some fixes for the
> examples in 2024 and would rather get agreement on the approach now than
> head down a wrong approach. Personally, I'd rather avoid #1, and #3 is
> neat, but perhaps being overly smart/complicated?
> 
> Regards,
> /Bruce

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

* Re: Updating examples which use coremask parameters
  2023-11-03 10:11 ` Konstantin Ananyev
@ 2023-11-03 10:16   ` Bruce Richardson
  2023-11-06 21:37     ` Konstantin Ananyev
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2023-11-03 10:16 UTC (permalink / raw)
  To: Konstantin Ananyev; +Cc: dev, techboard, Euan Bourke

On Fri, Nov 03, 2023 at 10:11:08AM +0000, Konstantin Ananyev wrote:
> 
> > Hi all,
> > 
> > looking to start a discussion and get some input here.
> > 
> > There are a number of our examples in DPDK which still track core usage via
> > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> > "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> > former is a good example (or bad example depending on your viewpoint) of
> > this as it takes multiple coremask parameters - for RX cores, for TX cores,
> > for worker cores and optionally for scheduler cores.
> > 
> > Now, the simple solution to this is to just expand the 64-bit bitmask to
> > 128 bit or more, but I think that is just making things harder for the
> > user, since dealing with long bitmasks is very awkward and unwieldy. Better
> > instead to convert all examples using coremasks to using core lists
> > instead.
> > 
> > First step should be to take our EAL corelist processing code and refactor
> > it into a function that can be made public, so that it can be used by all
> > apps for parsing core lists. Simple enough!
> > 
> > The next part I'm looking for input on is - how do we switch the apps from
> > coremasks to core lists? Some options:
> > 
> > 1. Add in new commandline parameters for each app to work with core lists.
> >   This is what we did in the past with EAL, by adding -l as a replacement
> >   for -c. The advantage is that we maintain backward compatibility, but the
> >   downside is that it becomes hard to find new suitable letter options for
> >   the core lists. Taking eventdev_pipeline again, we would need "new"
> >   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
> >   versions of these would be a simple alternative, but "-W" is already used
> >   as an app parameter so we can't do that.
> > 
> > 2. Just break backward compatibility and switch the apps to taking
> >   core lists instead of masks. Advantage is that it gives us the cleanest
> >   solution, but the downside is that and testing done using these examples,
> >   or any users who may have run them in the past, get different behaviour.
> 
> 
> As it is for examples, I also don't see any issue with converting to core-list.
> Said that, I suppose we still want to keep EAL '-c' (coremask) parameter, right?
> If so, then it might be plausible to consider making the code that handles it
> to work with really-long ones (up to 1K, or whatever is our current CPU_SET limit).

I believe the EAL coremask parsing already supports >64 lcores, and works
with arbitrary lengths up to RTE_MAX_LCORE, so I think we are ok here. It
parses the coremask char-by-char (backwards) as a string, rather than
trying to convert it using atoi-type functions[1].

/Bruce

[1] http://git.dpdk.org/dpdk/tree/lib/eal/common/eal_common_options.c#n777

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

* Re: Updating examples which use coremask parameters
  2023-11-02 16:58   ` Bruce Richardson
@ 2023-11-06 19:19     ` Stephen Hemminger
  0 siblings, 0 replies; 10+ messages in thread
From: Stephen Hemminger @ 2023-11-06 19:19 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: Thomas Monjalon, dev, techboard, Euan Bourke

On Thu, 2 Nov 2023 16:58:52 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Nov 02, 2023 at 05:28:42PM +0100, Thomas Monjalon wrote:
> > 02/11/2023 15:56, Bruce Richardson:  
> > > Hi all,
> > > 
> > > looking to start a discussion and get some input here.
> > > 
> > > There are a number of our examples in DPDK which still track core usage via
> > > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > > RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> > > "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> > > former is a good example (or bad example depending on your viewpoint) of
> > > this as it takes multiple coremask parameters - for RX cores, for TX cores,
> > > for worker cores and optionally for scheduler cores.
> > > 
> > > Now, the simple solution to this is to just expand the 64-bit bitmask to
> > > 128 bit or more, but I think that is just making things harder for the
> > > user, since dealing with long bitmasks is very awkward and unwieldy. Better
> > > instead to convert all examples using coremasks to using core lists
> > > instead.
> > > 
> > > First step should be to take our EAL corelist processing code and refactor
> > > it into a function that can be made public, so that it can be used by all
> > > apps for parsing core lists. Simple enough!  
> > 
> > OK to add some command line parsing helpers.
> > It should probably be the start of a new library for command line.
> >   
> 
> Funnily enough, separate to this I had already been working on an
> "rte_args" library to have some functions for working on argc/argv
> parameters. I'm planning on pushing out an RFC for 24.03 fairly shortly.
> 
> However, pulling in functions for arg parsing is a different set of
> functionality to what I had in mind, so we may yet get two libraries out of
> this. [Merging may be tricky due to issues around circular dependencies
> with EAL. My arg management library is designed to "sit on top of" EAL,
> while any lib offering e.g. coremask, corelist parsing functions would need
> to "sit beneath" EAL, so EAL can re-use it's functions].
> 
> Let's postpone the details of both these to when we get some RFCs out
> though.
> 
> 
> > > The next part I'm looking for input on is - how do we switch the apps from
> > > coremasks to core lists? Some options:
> > > 
> > > 1. Add in new commandline parameters for each app to work with core lists.
> > >   This is what we did in the past with EAL, by adding -l as a replacement
> > >   for -c. The advantage is that we maintain backward compatibility, but the
> > >   downside is that it becomes hard to find new suitable letter options for
> > >   the core lists. Taking eventdev_pipeline again, we would need "new"
> > >   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
> > >   versions of these would be a simple alternative, but "-W" is already used
> > >   as an app parameter so we can't do that.
> > > 
> > > 2. Just break backward compatibility and switch the apps to taking
> > >   core lists instead of masks. Advantage is that it gives us the cleanest
> > >   solution, but the downside is that and testing done using these examples,
> > >   or any users who may have run them in the past, get different behaviour.  
> > 
> > We don't need to offer backward compatibility in examples.
> > So this option is acceptable.
> >   
> 
> Glad to hear it. Makes life simpler.
> 
> > > 3. An interesting further alternative is to allow apps to take both
> > > coremasks and corelists and use heuristics to determine which is which.
> > > For example, anything starting with "0x" is a mask, anything containing
> > > "-" or "," is a list. There would be ambiguous values such as e.g. 2,
> > > which could be either, but we can always find ways to disambiguate
> > > these, e.g. allow trailing commas in lists, so that "0x2" is the
> > > coremask, and "2," is the corelist. [Could be other alternatives]. This
> > > largely keeps backward compatibility and also allows use of corelists.  
> > 
> > The option 3 can be interesting as well.
> >  
> Yep. If we start offering a library of arg-parsing functions, one of those
> could be a function using heuristics to identify core-mask, core-list or
> ambiguous values. Then each app can decide what to do in the latter case.
> Since we don't care about backward compatibility, the examples can just parse
> ambiguous values as core-list. User could then still use coremasks by
> prefixing them with "0x".

Noticed that lib/graph is using 64 bit coremask internally.
Wonder if others have the same issue.
Would be good if DPDK had a library to handle cpusets better, something
like the Linux kernel cpuset which uses comma separated list of cpu masks.

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

* RE: Updating examples which use coremask parameters
  2023-11-03 10:16   ` Bruce Richardson
@ 2023-11-06 21:37     ` Konstantin Ananyev
  2023-11-07  9:50       ` Bruce Richardson
  0 siblings, 1 reply; 10+ messages in thread
From: Konstantin Ananyev @ 2023-11-06 21:37 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, techboard, Euan Bourke


> > >
> > > looking to start a discussion and get some input here.
> > >
> > > There are a number of our examples in DPDK which still track core usage via
> > > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > > RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> > > "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> > > former is a good example (or bad example depending on your viewpoint) of
> > > this as it takes multiple coremask parameters - for RX cores, for TX cores,
> > > for worker cores and optionally for scheduler cores.
> > >
> > > Now, the simple solution to this is to just expand the 64-bit bitmask to
> > > 128 bit or more, but I think that is just making things harder for the
> > > user, since dealing with long bitmasks is very awkward and unwieldy. Better
> > > instead to convert all examples using coremasks to using core lists
> > > instead.
> > >
> > > First step should be to take our EAL corelist processing code and refactor
> > > it into a function that can be made public, so that it can be used by all
> > > apps for parsing core lists. Simple enough!
> > >
> > > The next part I'm looking for input on is - how do we switch the apps from
> > > coremasks to core lists? Some options:
> > >
> > > 1. Add in new commandline parameters for each app to work with core lists.
> > >   This is what we did in the past with EAL, by adding -l as a replacement
> > >   for -c. The advantage is that we maintain backward compatibility, but the
> > >   downside is that it becomes hard to find new suitable letter options for
> > >   the core lists. Taking eventdev_pipeline again, we would need "new"
> > >   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
> > >   versions of these would be a simple alternative, but "-W" is already used
> > >   as an app parameter so we can't do that.
> > >
> > > 2. Just break backward compatibility and switch the apps to taking
> > >   core lists instead of masks. Advantage is that it gives us the cleanest
> > >   solution, but the downside is that and testing done using these examples,
> > >   or any users who may have run them in the past, get different behaviour.
> >
> >
> > As it is for examples, I also don't see any issue with converting to core-list.
> > Said that, I suppose we still want to keep EAL '-c' (coremask) parameter, right?
> > If so, then it might be plausible to consider making the code that handles it
> > to work with really-long ones (up to 1K, or whatever is our current CPU_SET limit).
> 
> I believe the EAL coremask parsing already supports >64 lcores, and works
> with arbitrary lengths up to RTE_MAX_LCORE, so I think we are ok here. It
> parses the coremask char-by-char (backwards) as a string, rather than
> trying to convert it using atoi-type functions[1].

Great, thanks for clarifying.
I suppose next question here -  would it make sense to convert that code into some public API,
so we can have one function for  core-mask parsing that will be used for both EAL and user apps? 
 
> 
> /Bruce
> 
> [1] http://git.dpdk.org/dpdk/tree/lib/eal/common/eal_common_options.c#n777


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

* Re: Updating examples which use coremask parameters
  2023-11-06 21:37     ` Konstantin Ananyev
@ 2023-11-07  9:50       ` Bruce Richardson
  0 siblings, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2023-11-07  9:50 UTC (permalink / raw)
  To: Konstantin Ananyev; +Cc: dev, techboard, Euan Bourke

On Mon, Nov 06, 2023 at 09:37:31PM +0000, Konstantin Ananyev wrote:
> 
> > > >
> > > > looking to start a discussion and get some input here.
> > > >
> > > > There are a number of our examples in DPDK which still track core usage via
> > > > a 64-bit bitmask, and, as such, cannot run on cores between 64 and
> > > > RTE_MAX_LCORE. Two examples I have recently come across with this issue are
> > > > "eventdev_pipeline" and "qos_sched", but I am sure there are others. The
> > > > former is a good example (or bad example depending on your viewpoint) of
> > > > this as it takes multiple coremask parameters - for RX cores, for TX cores,
> > > > for worker cores and optionally for scheduler cores.
> > > >
> > > > Now, the simple solution to this is to just expand the 64-bit bitmask to
> > > > 128 bit or more, but I think that is just making things harder for the
> > > > user, since dealing with long bitmasks is very awkward and unwieldy. Better
> > > > instead to convert all examples using coremasks to using core lists
> > > > instead.
> > > >
> > > > First step should be to take our EAL corelist processing code and refactor
> > > > it into a function that can be made public, so that it can be used by all
> > > > apps for parsing core lists. Simple enough!
> > > >
> > > > The next part I'm looking for input on is - how do we switch the apps from
> > > > coremasks to core lists? Some options:
> > > >
> > > > 1. Add in new commandline parameters for each app to work with core lists.
> > > >   This is what we did in the past with EAL, by adding -l as a replacement
> > > >   for -c. The advantage is that we maintain backward compatibility, but the
> > > >   downside is that it becomes hard to find new suitable letter options for
> > > >   the core lists. Taking eventdev_pipeline again, we would need "new"
> > > >   options for "-r", "-t", "-w" and "-s" parameters. Using the capitalized
> > > >   versions of these would be a simple alternative, but "-W" is already used
> > > >   as an app parameter so we can't do that.
> > > >
> > > > 2. Just break backward compatibility and switch the apps to taking
> > > >   core lists instead of masks. Advantage is that it gives us the cleanest
> > > >   solution, but the downside is that and testing done using these examples,
> > > >   or any users who may have run them in the past, get different behaviour.
> > >
> > >
> > > As it is for examples, I also don't see any issue with converting to core-list.
> > > Said that, I suppose we still want to keep EAL '-c' (coremask) parameter, right?
> > > If so, then it might be plausible to consider making the code that handles it
> > > to work with really-long ones (up to 1K, or whatever is our current CPU_SET limit).
> > 
> > I believe the EAL coremask parsing already supports >64 lcores, and works
> > with arbitrary lengths up to RTE_MAX_LCORE, so I think we are ok here. It
> > parses the coremask char-by-char (backwards) as a string, rather than
> > trying to convert it using atoi-type functions[1].
> 
> Great, thanks for clarifying.
> I suppose next question here -  would it make sense to convert that code into some public API,
> so we can have one function for  core-mask parsing that will be used for both EAL and user apps? 
>  

Yes, that is the intention.

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

end of thread, other threads:[~2023-11-07  9:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-02 14:56 Updating examples which use coremask parameters Bruce Richardson
2023-11-02 16:28 ` Thomas Monjalon
2023-11-02 16:58   ` Bruce Richardson
2023-11-06 19:19     ` Stephen Hemminger
2023-11-02 17:05 ` Morten Brørup
2023-11-02 17:15   ` Bruce Richardson
2023-11-03 10:11 ` Konstantin Ananyev
2023-11-03 10:16   ` Bruce Richardson
2023-11-06 21:37     ` Konstantin Ananyev
2023-11-07  9:50       ` Bruce Richardson

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).