* [RFC PATCH v3 0/2] xen: credit2: flexible configuration for runqueues
@ 2017-03-30 19:28 Praveen Kumar
2017-03-30 19:28 ` [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation Praveen Kumar
2017-03-30 19:28 ` [RFC PATCH v3 2/2] xen: credit2: provide custom option to create Praveen Kumar
0 siblings, 2 replies; 6+ messages in thread
From: Praveen Kumar @ 2017-03-30 19:28 UTC (permalink / raw)
To: george.dunlap, dario.faggioli; +Cc: Praveen Kumar, xen-devel
Hello,
The idea is to give user more flexibility to configure runqueue further.
For most workloads and in most systems, using per-core means have too many
small runqueues. Using per-socket is almost always better, but it may result
in too few big runqueues.
OPTION 1 :
--------
The user can create runqueue per-cpu using Xen boot parameter like below:
credit2_runqueue=cpu
which would mean the following:
- pCPU 0 belong to runqueue 0
- pCPU 1 belong to runqueue 1
- pCPU 2 belong to runqueue 2
and so on.
OPTION 2 :
--------
Further user can be allowed to say something shown below :
credit2_runqueue=0,1,4,5;2,3,6,7;8,9,12,13;10,11,14,15
or (with exactly the same meaning, but a perhaps more clear syntax):
credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
which would mean the following:
- pCPUs 0, 1, 4 and 5 belong to runqueue 0
- pCPUs 2, 3, 6 and 7 belong to runqueue 1
- pCPUs 8, 9, 12 and 13 belong to runqueue 2
- pCPUs 10, 11, 14 and 15 belong to runqueue 3
Thanks and Regards,
Praveen Kumar.
---
v3 has proper subject for RFC
---
Praveen Kumar (2):
xen: credit2: enable per cpu runqueue creation
xen: credit2: provide flexible option to create runqueue
docs/misc/xen-command-line.markdown | 9 +-
xen/common/sched_credit2.c | 185 +++++++++++++++++++++++++++++++++++-
2 files changed, 188 insertions(+), 6 deletions(-)
--
2.12.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation
2017-03-30 19:28 [RFC PATCH v3 0/2] xen: credit2: flexible configuration for runqueues Praveen Kumar
@ 2017-03-30 19:28 ` Praveen Kumar
2017-03-31 7:18 ` Dario Faggioli
2017-03-30 19:28 ` [RFC PATCH v3 2/2] xen: credit2: provide custom option to create Praveen Kumar
1 sibling, 1 reply; 6+ messages in thread
From: Praveen Kumar @ 2017-03-30 19:28 UTC (permalink / raw)
To: george.dunlap, dario.faggioli; +Cc: Praveen Kumar, xen-devel
The patch introduces a new command line option 'cpu' that when used will
create runqueue per logical pCPU.
Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com>
---
docs/misc/xen-command-line.markdown | 3 ++-
xen/common/sched_credit2.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 9eb85d68b5..c245cfa471 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -525,7 +525,7 @@ also slow in responding to load changes.
The default value of `1 sec` is rather long.
### credit2\_runqueue
-> `= core | socket | node | all`
+> `= cpu | core | socket | node | all`
> Default: `socket`
@@ -536,6 +536,7 @@ balancing (for instance, it will deal better with hyperthreading),
but also more overhead.
Available alternatives, with their meaning, are:
+* `cpu`: one runqueue per each logical pCPUs of the host;
* `core`: one runqueue per each physical core of the host;
* `socket`: one runqueue per each physical socket (which often,
but not always, matches a NUMA node) of the host;
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index bb1c657e76..ee7b443f9e 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -301,6 +301,9 @@ integer_param("credit2_balance_over", opt_overload_balance_tolerance);
* want that to happen basing on topology. At the moment, it is possible
* to choose to arrange runqueues to be:
*
+ * - per-cpu: meaning that there will be one runqueue per logical cpu. This
+ * will happen when if the opt_runqueue parameter is set to 'cpu'.
+ *
* - per-core: meaning that there will be one runqueue per each physical
* core of the host. This will happen if the opt_runqueue
* parameter is set to 'core';
@@ -322,11 +325,13 @@ integer_param("credit2_balance_over", opt_overload_balance_tolerance);
* either the same physical core, the same physical socket, the same NUMA
* node, or just all of them, will be put together to form runqueues.
*/
-#define OPT_RUNQUEUE_CORE 0
-#define OPT_RUNQUEUE_SOCKET 1
-#define OPT_RUNQUEUE_NODE 2
-#define OPT_RUNQUEUE_ALL 3
+#define OPT_RUNQUEUE_CPU 0
+#define OPT_RUNQUEUE_CORE 1
+#define OPT_RUNQUEUE_SOCKET 2
+#define OPT_RUNQUEUE_NODE 3
+#define OPT_RUNQUEUE_ALL 4
static const char *const opt_runqueue_str[] = {
+ [OPT_RUNQUEUE_CPU] = "cpu",
[OPT_RUNQUEUE_CORE] = "core",
[OPT_RUNQUEUE_SOCKET] = "socket",
[OPT_RUNQUEUE_NODE] = "node",
@@ -682,6 +687,8 @@ cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
BUG_ON(cpu_to_socket(cpu) == XEN_INVALID_SOCKET_ID ||
cpu_to_socket(peer_cpu) == XEN_INVALID_SOCKET_ID);
+ if (opt_runqueue == OPT_RUNQUEUE_CPU)
+ continue;
if ( opt_runqueue == OPT_RUNQUEUE_ALL ||
(opt_runqueue == OPT_RUNQUEUE_CORE && same_core(peer_cpu, cpu)) ||
(opt_runqueue == OPT_RUNQUEUE_SOCKET && same_socket(peer_cpu, cpu)) ||
--
2.12.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH v3 2/2] xen: credit2: provide custom option to create
2017-03-30 19:28 [RFC PATCH v3 0/2] xen: credit2: flexible configuration for runqueues Praveen Kumar
2017-03-30 19:28 ` [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation Praveen Kumar
@ 2017-03-30 19:28 ` Praveen Kumar
2017-03-31 8:32 ` Dario Faggioli
1 sibling, 1 reply; 6+ messages in thread
From: Praveen Kumar @ 2017-03-30 19:28 UTC (permalink / raw)
To: george.dunlap, dario.faggioli; +Cc: Praveen Kumar, xen-devel
The patch introduces a new command line option 'custom' that when used will
create runqueue based upon the pCPU subset provide during bootup.
Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com>
---
docs/misc/xen-command-line.markdown | 8 +-
xen/common/sched_credit2.c | 170 +++++++++++++++++++++++++++++++++++-
2 files changed, 176 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index c245cfa471..e65056b3d0 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -525,7 +525,7 @@ also slow in responding to load changes.
The default value of `1 sec` is rather long.
### credit2\_runqueue
-> `= cpu | core | socket | node | all`
+> `= cpu | core | socket | node | all | custom`
> Default: `socket`
@@ -543,6 +543,12 @@ Available alternatives, with their meaning, are:
* `node`: one runqueue per each NUMA node of the host;
* `all`: just one runqueue shared by all the logical pCPUs of
the host
+* `custom`: one runqueue per subset. Example:
+ credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
+ - pCPUs 0, 1, 4 and 5 belong to runqueue 0
+ - pCPUs 2, 3, 6 and 7 belong to runqueue 1
+ - pCPUs 8, 9, 12 and 13 belong to runqueue 2
+ - pCPUs 10, 11, 14 and 15 belong to runqueue 3
### dbgp
> `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index ee7b443f9e..9234b023cb 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -321,6 +321,15 @@ integer_param("credit2_balance_over", opt_overload_balance_tolerance);
* (logical) processors of the host belong. This will happen if
* the opt_runqueue parameter is set to 'all'.
*
+ * - custom: meaning that there will be one runqueue per subset being passed as
+ * parameter to credit2_runqueue as shown in below example.
+ * Example:
+ * credit2_runqueue=[[cpu0,cpu1][cpu3][cpu4,cpu5]]
+ * The example mentioned states :
+ * cpu0 and cpu1 belongs to runqueue 0
+ * cpu3 belongs to runqueue 1
+ * cpu4 and cpu5 belongs to runqueue 2
+ *
* Depending on the value of opt_runqueue, therefore, cpus that are part of
* either the same physical core, the same physical socket, the same NUMA
* node, or just all of them, will be put together to form runqueues.
@@ -330,18 +339,60 @@ integer_param("credit2_balance_over", opt_overload_balance_tolerance);
#define OPT_RUNQUEUE_SOCKET 2
#define OPT_RUNQUEUE_NODE 3
#define OPT_RUNQUEUE_ALL 4
+#define OPT_RUNQUEUE_CUSTOM 5
static const char *const opt_runqueue_str[] = {
[OPT_RUNQUEUE_CPU] = "cpu",
[OPT_RUNQUEUE_CORE] = "core",
[OPT_RUNQUEUE_SOCKET] = "socket",
[OPT_RUNQUEUE_NODE] = "node",
- [OPT_RUNQUEUE_ALL] = "all"
+ [OPT_RUNQUEUE_ALL] = "all",
+ [OPT_RUNQUEUE_CUSTOM] = "custom"
};
static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
+static int __read_mostly custom_cpu_runqueue[NR_CPUS];
+
+#define GETTOKEN( token, len, start, end ) \
+{ \
+ char _tmp[len+1]; \
+ int _i; \
+ safe_strcpy(_tmp, start); \
+ _tmp[len] = '\0'; \
+ for ( _i = 0; _tmp[_i] != '\0'; _i++ ) \
+ token = ( ( token * 10 ) + ( _tmp[_i] - '0' ) ); \
+}
+
+static inline int trim(const char *c, char *t, char elem)
+{
+ int l = strlen(c);
+ const char *x = c ;
+ int i = 0;
+ if ( !c || !t )
+ return -1;
+ while ( *x != '\0' && i < l )
+ {
+ if ( *x != elem )
+ t[i++] = *x;
+ x++;
+ }
+ t[i] = '\0';
+ return 0;
+}
+
+static inline int getlen(char *start, char *end)
+{
+ if ( ( start ) && ( end ) && ( end > start ) )
+ return end-start;
+ else
+ return -1;
+}
+
static void parse_credit2_runqueue(const char *s)
{
unsigned int i;
+ const char *s_end = NULL;
+ char m[strlen(s)];
+ char *_s = NULL;
for ( i = 0; i < ARRAY_SIZE(opt_runqueue_str); i++ )
{
@@ -351,7 +402,115 @@ static void parse_credit2_runqueue(const char *s)
return;
}
}
+/*
+ * At this stage we are either unknown value of credit2_runqueue or we can
+ * consider it to be custom cpu. Lets try parsing the same.
+ * Resetting the custom_cpu_runqueue for future use. Only the non-negative
+ * entries will be valid. The index 'i' in custom_cpu_runqueue will store
+ * the specific runqueue it belongs to.
+ * Example:
+ * If custom_cpu_runqueue[3] == 2
+ * Then, it means that cpu 3 belong to runqueue 2.
+ * If custom_cpu_runqueue[4] == -1
+ * Then, it means that cpu 4 doesn't belong to any runqueue.
+ */
+ for ( i = 0; i < nr_cpu_ids; i++ )
+ custom_cpu_runqueue[i] = -1;
+
+ /*
+ * Format [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
+ */
+ i = 0;
+
+ /* In case user have spaces included in the input */
+ if ( trim(s, m, ' ') != 0 )
+ {
+ printk( "WARNING : %s[%d] trim failed.\n", __func__, __LINE__ );
+ goto errReturn;
+ }
+ /* Starting to parse and get the cpu information on trimmed data */
+ _s = m;
+ s_end = _s + strlen(_s);
+
+ /* The start and should always be in format of '[..]' */
+ if ( ( '[' == *_s ) && ( ']' == *(s_end-1)) )
+ {
+ char *start = NULL, *end = NULL;
+ int cpu_added_to_runqueue = 0;
+ _s++;
+ while ( ( _s != NULL ) && ( _s < s_end ) )
+ {
+ char *token_sub_str = NULL;
+ start = strstr(_s, "[");
+ end = strstr(_s, "]");
+ /* Validation checks for '[' and ']' to properly parse the entries
+ */
+ if ( ( !start && !end ) || ( ( end == ( s_end -1 ) ) && start ) )
+ goto errReturn;
+ /* Check for last entry */
+ else if ( !start && ( end == ( s_end -1 ) ) )
+ goto nextSet;
+
+ /* If we have [] as entry, move to nextSet */
+ if ( getlen ( start, end ) < 1 )
+ goto nextSet;
+ /* Start to parse the actual value */
+ start++;
+ /*
+ * find token within the subset
+ */
+ do
+ {
+ int token = 0;
+ int len = 0;
+ /* Get cpu ids separated by ',' within each set */
+ token_sub_str = strpbrk(start, ",");
+ if ( ( !token_sub_str && start < end ) ||
+ ( token_sub_str > end && token_sub_str > start ) )
+ len = getlen(start, end);
+ else
+ len = getlen(start, token_sub_str);
+
+ if ( len <= 0 )
+ continue;
+ /* GETTOKEN will get return the parse and populate the cpu in
+ * token
+ */
+ GETTOKEN(token, len, start , end );
+
+ if ( token >= nr_cpu_ids)
+ goto errReturn;
+ /* If not set already */
+ if ( custom_cpu_runqueue[token] == -1 )
+ {
+ custom_cpu_runqueue[token] = i;
+ cpu_added_to_runqueue = 1;
+ }
+ else
+ goto errReturn;
+
+ if ( !token_sub_str || token_sub_str > end )
+ goto nextSet;
+
+ start = ++token_sub_str;
+ } while ( start < end );
+nextSet:
+ if ( cpu_added_to_runqueue )
+ {
+ i++;
+ cpu_added_to_runqueue = 0;
+ }
+ _s = ++end;
+ }
+ opt_runqueue = OPT_RUNQUEUE_CUSTOM;
+ return;
+ }
+errReturn:
+ /* Resetting in case of failure, so that we don't mess-up during any failure
+ * due to wrong or spurious pattern passed by user.
+ */
+ opt_runqueue = OPT_RUNQUEUE_SOCKET;
printk("WARNING, unrecognized value of credit2_runqueue option!\n");
}
custom_param("credit2_runqueue", parse_credit2_runqueue);
@@ -661,6 +820,15 @@ cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
struct csched2_runqueue_data *rqd;
unsigned int rqi;
+ if ( opt_runqueue == OPT_RUNQUEUE_CUSTOM )
+ {
+ if ( custom_cpu_runqueue[cpu] != -1 )
+ {
+ BUG_ON(custom_cpu_runqueue[cpu] >= nr_cpu_ids);
+ return custom_cpu_runqueue[cpu];
+ }
+ }
+
for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
{
unsigned int peer_cpu;
--
2.12.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation
2017-03-30 19:28 ` [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation Praveen Kumar
@ 2017-03-31 7:18 ` Dario Faggioli
0 siblings, 0 replies; 6+ messages in thread
From: Dario Faggioli @ 2017-03-31 7:18 UTC (permalink / raw)
To: Praveen Kumar, george.dunlap; +Cc: xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 857 bytes --]
Hello Praveen,
On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> The patch introduces a new command line option 'cpu' that when used
> will
> create runqueue per logical pCPU.
>
I'd add a quick note about why we think it's a good idea to have this.
Something like:
"This may be useful for small systems, and also for development,
performance evaluation and comparison."
> Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com>
>
With that addition, at least this patch can have my:
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 2/2] xen: credit2: provide custom option to create
2017-03-30 19:28 ` [RFC PATCH v3 2/2] xen: credit2: provide custom option to create Praveen Kumar
@ 2017-03-31 8:32 ` Dario Faggioli
2017-04-14 18:14 ` Praveen Kumar
0 siblings, 1 reply; 6+ messages in thread
From: Dario Faggioli @ 2017-03-31 8:32 UTC (permalink / raw)
To: Praveen Kumar, george.dunlap; +Cc: xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 12686 bytes --]
On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> The patch introduces a new command line option 'custom' that when
used will
> create runqueue based upon the pCPU subset provide during bootup.
>
"introduces a new, very flexible, way of arranging runqueues in
Credit2. It allows to specify, explicitly and precisely, what pCPUs
should belong to which runqueue"
Or something like this.
It's great that you've got the code working. I have some comments,
though, on how it actually looks like.
> Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com>
> ---
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -525,7 +525,7 @@ also slow in responding to load changes.
> The default value of `1 sec` is rather long.
>
> ### credit2\_runqueue
> -> `= cpu | core | socket | node | all`
> +> `= cpu | core | socket | node | all | custom`
>
Err... but this is not really correct, right?
I mean, it's not that the user should use the word 'custom' here.
You probably want to use something like <custom>, and then define what
that means below.
> @@ -543,6 +543,12 @@ Available alternatives, with their meaning, are:
> * `node`: one runqueue per each NUMA node of the host;
> * `all`: just one runqueue shared by all the logical pCPUs of
> the host
> +* `custom`: one runqueue per subset. Example:
> + credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14
> ,15]]
>
Maybe just use 2 (or at most 3) pCPUs per runqueue in the example. It'd
make the line shorter and easier to read and understand.
> + - pCPUs 0, 1, 4 and 5 belong to runqueue 0
> + - pCPUs 2, 3, 6 and 7 belong to runqueue 1
> + - pCPUs 8, 9, 12 and 13 belong to runqueue 2
> + - pCPUs 10, 11, 14 and 15 belong to runqueue 3
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -330,18 +339,60 @@ integer_param("credit2_balance_over",
> opt_overload_balance_tolerance);
> #define OPT_RUNQUEUE_SOCKET 2
> #define OPT_RUNQUEUE_NODE 3
> #define OPT_RUNQUEUE_ALL 4
> +#define OPT_RUNQUEUE_CUSTOM 5
> static const char *const opt_runqueue_str[] = {
> [OPT_RUNQUEUE_CPU] = "cpu",
> [OPT_RUNQUEUE_CORE] = "core",
> [OPT_RUNQUEUE_SOCKET] = "socket",
> [OPT_RUNQUEUE_NODE] = "node",
> - [OPT_RUNQUEUE_ALL] = "all"
> + [OPT_RUNQUEUE_ALL] = "all",
> + [OPT_RUNQUEUE_CUSTOM] = "custom"
> };
> static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
>
> +static int __read_mostly custom_cpu_runqueue[NR_CPUS];
> +
I think this can be nr_cpu_ids (and be allocated dynamically during
parsing).
> +#define GETTOKEN( token, len, start, end ) \
> +{ \
> + char _tmp[len+1]; \
> + int _i; \
> + safe_strcpy(_tmp, start); \
> + _tmp[len] = '\0'; \
> + for ( _i = 0; _tmp[_i] != '\0'; _i++ ) \
> + token = ( ( token * 10 ) + ( _tmp[_i] - '0' ) ); \
> +}
> +
If you really need this, make it 'static inline', as you do for other
functions.
As a matter of fact, I don't think you need it, as, for instance, we do
have simple_strtoul() (and some others). :-)
> +static inline int trim(const char *c, char *t, char elem)
> +{
> + int l = strlen(c);
> + const char *x = c ;
> + int i = 0;
> + if ( !c || !t )
> + return -1;
> + while ( *x != '\0' && i < l )
> + {
> + if ( *x != elem )
> + t[i++] = *x;
> + x++;
> + }
> + t[i] = '\0';
> + return 0;
> +}
> +
Again, why we need this? Can't we just deal with invalid characters
while parsing the string (by, e.g., either skipping them or aborting,
depending on whether or not they're innocuous)?
> +static inline int getlen(char *start, char *end)
> +{
> + if ( ( start ) && ( end ) && ( end > start ) )
> + return end-start;
> + else
> + return -1;
> +}
> +
>
Same.
> static void parse_credit2_runqueue(const char *s)
> {
> unsigned int i;
> + const char *s_end = NULL;
> + char m[strlen(s)];
> + char *_s = NULL;
>
No '_' prefixed variable names, please. :-)
Also, what's m for?
> @@ -351,7 +402,115 @@ static void parse_credit2_runqueue(const char
> *s)
> return;
> }
> }
> +/*
> + * At this stage we are either unknown value of credit2_runqueue
> or we can
> + * consider it to be custom cpu. Lets try parsing the same.
> + * Resetting the custom_cpu_runqueue for future use. Only the
> non-negative
> + * entries will be valid. The index 'i' in custom_cpu_runqueue
> will store
> + * the specific runqueue it belongs to.
> + * Example:
> + * If custom_cpu_runqueue[3] == 2
> + * Then, it means that cpu 3 belong to runqueue 2.
> + * If custom_cpu_runqueue[4] == -1
> + * Then, it means that cpu 4 doesn't belong to any runqueue.
> + */
> + for ( i = 0; i < nr_cpu_ids; i++ )
> + custom_cpu_runqueue[i] = -1;
> +
> + /*
> + * Format [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
> + */
> + i = 0;
> +
> + /* In case user have spaces included in the input */
> + if ( trim(s, m, ' ') != 0 )
> + {
> + printk( "WARNING : %s[%d] trim failed.\n", __func__,
> __LINE__ );
> + goto errReturn;
> + }
>
Right. So, as I was saying above, can't we just start parsing the
string --with something like a `char *p; while ( *p != '\0' ) {...}`--
and skip the spaces when we find them?
> + /* Starting to parse and get the cpu information on trimmed data
> */
> + _s = m;
> + s_end = _s + strlen(_s);
> +
> + /* The start and should always be in format of '[..]' */
> + if ( ( '[' == *_s ) && ( ']' == *(s_end-1)) )
>
But in the cover you said we also support:
credit2_runqueue=0,1,4,5;2,3,6,7;8,9,12,13;10,11,14,15
So, what I think we want here is something that checks that, if the
first character is '[', then the last must be ']', or we already know
the format is invalid, and we can stop.
And it would also be better to write an if that checks whether the
format is _invalid_, and stop the parsing if that is the case. That
saves a level of indentation for all the code that follows.
> + {
> + char *start = NULL, *end = NULL;
> + int cpu_added_to_runqueue = 0;
>
It looks like this can be bool.
> + _s++;
> + while ( ( _s != NULL ) && ( _s < s_end ) )
> + {
> + char *token_sub_str = NULL;
>
We want a blank line between variable definition and actual code.
> + start = strstr(_s, "[");
> + end = strstr(_s, "]");
> + /* Validation checks for '[' and ']' to properly parse
> the entries
> + */
> + if ( ( !start && !end ) || ( ( end == ( s_end -1 ) ) &&
> start ) )
> + goto errReturn;
>
No camel case either, please. 'err' is probably just fine. :-)
But, honestly, I can't quite tell what's going on here. Why strstr()?
You can just check whether the first char of a set is '[' by looking at
the current string parsing cursor. If it is, you can take a note about
that somewhere, to remember to check for a ']' at some point.
> + /* Check for last entry */
> + else if ( !start && ( end == ( s_end -1 ) ) )
> + goto nextSet;
> +
> + /* If we have [] as entry, move to nextSet */
> + if ( getlen ( start, end ) < 1 )
> + goto nextSet;
>
'next' (or 'next_set', but I think 'next' is fine).
> + /* Start to parse the actual value */
> + start++;
> + /*
> + * find token within the subset
> + */
> + do
> + {
> + int token = 0;
> + int len = 0;
> + /* Get cpu ids separated by ',' within each set */
> + token_sub_str = strpbrk(start, ",");
> + if ( ( !token_sub_str && start < end ) ||
> + ( token_sub_str > end && token_sub_str > start )
> )
> + len = getlen(start, end);
> + else
> + len = getlen(start, token_sub_str);
> +
> + if ( len <= 0 )
> + continue;
> + /* GETTOKEN will get return the parse and populate
> the cpu in
> + * token
> + */
> + GETTOKEN(token, len, start , end );
> +
> + if ( token >= nr_cpu_ids)
> + goto errReturn;
> + /* If not set already */
> + if ( custom_cpu_runqueue[token] == -1 )
> + {
> + custom_cpu_runqueue[token] = i;
> + cpu_added_to_runqueue = 1;
> + }
> + else
> + goto errReturn;
> +
> + if ( !token_sub_str || token_sub_str > end )
> + goto nextSet;
> +
> + start = ++token_sub_str;
> + } while ( start < end );
> +nextSet:
> + if ( cpu_added_to_runqueue )
> + {
> + i++;
> + cpu_added_to_runqueue = 0;
> + }
>
>
> + _s = ++end;
> + }
>
Ok, so this looks more complex that it could be, to me. Mostly because
you're doing your own token parsing.
In my mind, parsing a string like the ones we want to accept, should
look not too different from what's done inside init_boot_pages().
Indeed our case looks a bit more complicated, but the general idea
should be the same, i.e.:
- scan the string and deal with what you find in there online, not
beforehand;
- use simple_strtoul()
> @@ -661,6 +820,15 @@ cpu_to_runqueue(struct csched2_private *prv,
> unsigned int cpu)
> struct csched2_runqueue_data *rqd;
> unsigned int rqi;
>
> + if ( opt_runqueue == OPT_RUNQUEUE_CUSTOM )
> + {
> + if ( custom_cpu_runqueue[cpu] != -1 )
> + {
> + BUG_ON(custom_cpu_runqueue[cpu] >= nr_cpu_ids);
> + return custom_cpu_runqueue[cpu];
> + }
>
Wait, and then what happens if a CPU was not in any set, and hence has
it's custom_cpu_runqueue element == -1 ?
I am ok with it not being added to any runqueue, but for that to be
what really happens, I think we need to change code in init_pdata()
too.
It would be also good to print a warning, in such case. This is a
custom mode, and the user may well be doing such a thing for a reason,
but it doesn't harm trying make sure nothing this bad happened by
mistake.
Thanks again for all this work! :-D
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH v3 2/2] xen: credit2: provide custom option to create
2017-03-31 8:32 ` Dario Faggioli
@ 2017-04-14 18:14 ` Praveen Kumar
0 siblings, 0 replies; 6+ messages in thread
From: Praveen Kumar @ 2017-04-14 18:14 UTC (permalink / raw)
To: Dario Faggioli; +Cc: george.dunlap, xen-devel
On Fri, Mar 31, 2017 at 10:32:09AM +0200, Dario Faggioli wrote:
> On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> > The patch introduces a new command line option 'custom' that when
> used will
> > create runqueue based upon the pCPU subset provide during bootup.
> >
> "introduces a new, very flexible, way of arranging runqueues in
> Credit2. It allows to specify, explicitly and precisely, what pCPUs
> should belong to which runqueue"
>
> Or something like this.
>
> It's great that you've got the code working. I have some comments,
> though, on how it actually looks like.
>
> > Signed-off-by: Praveen Kumar <kpraveen.lkml@gmail.com>
> > ---
>
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -525,7 +525,7 @@ also slow in responding to load changes.
> > The default value of `1 sec` is rather long.
> >
> > ### credit2\_runqueue
> > -> `= cpu | core | socket | node | all`
> > +> `= cpu | core | socket | node | all | custom`
> >
> Err... but this is not really correct, right?
>
> I mean, it's not that the user should use the word 'custom' here.
>
> You probably want to use something like <custom>, and then define what
> that means below.
>
> > @@ -543,6 +543,12 @@ Available alternatives, with their meaning, are:
> > * `node`: one runqueue per each NUMA node of the host;
> > * `all`: just one runqueue shared by all the logical pCPUs of
> > the host
> > +* `custom`: one runqueue per subset. Example:
> > + credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14
> > ,15]]
> >
> Maybe just use 2 (or at most 3) pCPUs per runqueue in the example. It'd
> make the line shorter and easier to read and understand.
>
Sure, Will take care of the same.
> > + - pCPUs 0, 1, 4 and 5 belong to runqueue 0
> > + - pCPUs 2, 3, 6 and 7 belong to runqueue 1
> > + - pCPUs 8, 9, 12 and 13 belong to runqueue 2
> > + - pCPUs 10, 11, 14 and 15 belong to runqueue 3
>
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -330,18 +339,60 @@ integer_param("credit2_balance_over",
> > opt_overload_balance_tolerance);
> > #define OPT_RUNQUEUE_SOCKET 2
> > #define OPT_RUNQUEUE_NODE 3
> > #define OPT_RUNQUEUE_ALL 4
> > +#define OPT_RUNQUEUE_CUSTOM 5
> > static const char *const opt_runqueue_str[] = {
> > [OPT_RUNQUEUE_CPU] = "cpu",
> > [OPT_RUNQUEUE_CORE] = "core",
> > [OPT_RUNQUEUE_SOCKET] = "socket",
> > [OPT_RUNQUEUE_NODE] = "node",
> > - [OPT_RUNQUEUE_ALL] = "all"
> > + [OPT_RUNQUEUE_ALL] = "all",
> > + [OPT_RUNQUEUE_CUSTOM] = "custom"
> > };
> > static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
> >
> > +static int __read_mostly custom_cpu_runqueue[NR_CPUS];
> > +
> I think this can be nr_cpu_ids (and be allocated dynamically during
> parsing).
>
Had to use NR_CPUS to make compiler happy with "error:Variably modified 'types'
at file scope".
> > +#define GETTOKEN( token, len, start, end ) \
> > +{ \
> > + char _tmp[len+1]; \
> > + int _i; \
> > + safe_strcpy(_tmp, start); \
> > + _tmp[len] = '\0'; \
> > + for ( _i = 0; _tmp[_i] != '\0'; _i++ ) \
> > + token = ( ( token * 10 ) + ( _tmp[_i] - '0' ) ); \
> > +}
> > +
> If you really need this, make it 'static inline', as you do for other
> functions.
>
> As a matter of fact, I don't think you need it, as, for instance, we do
> have simple_strtoul() (and some others). :-)
>
True, will be replacing this with simple_strtoul()
> > +static inline int trim(const char *c, char *t, char elem)
> > +{
> > + int l = strlen(c);
> > + const char *x = c ;
> > + int i = 0;
> > + if ( !c || !t )
> > + return -1;
> > + while ( *x != '\0' && i < l )
> > + {
> > + if ( *x != elem )
> > + t[i++] = *x;
> > + x++;
> > + }
> > + t[i] = '\0';
> > + return 0;
> > +}
> > +
> Again, why we need this? Can't we just deal with invalid characters
> while parsing the string (by, e.g., either skipping them or aborting,
> depending on whether or not they're innocuous)?
>
> > +static inline int getlen(char *start, char *end)
> > +{
> > + if ( ( start ) && ( end ) && ( end > start ) )
> > + return end-start;
> > + else
> > + return -1;
> > +}
> > +
> >
> Same.
>
> > static void parse_credit2_runqueue(const char *s)
> > {
> > unsigned int i;
> > + const char *s_end = NULL;
> > + char m[strlen(s)];
> > + char *_s = NULL;
> >
> No '_' prefixed variable names, please. :-)
>
> Also, what's m for?
>
> > @@ -351,7 +402,115 @@ static void parse_credit2_runqueue(const char
> > *s)
> > return;
> > }
> > }
> > +/*
> > + * At this stage we are either unknown value of credit2_runqueue
> > or we can
> > + * consider it to be custom cpu. Lets try parsing the same.
> > + * Resetting the custom_cpu_runqueue for future use. Only the
> > non-negative
> > + * entries will be valid. The index 'i' in custom_cpu_runqueue
> > will store
> > + * the specific runqueue it belongs to.
> > + * Example:
> > + * If custom_cpu_runqueue[3] == 2
> > + * Then, it means that cpu 3 belong to runqueue 2.
> > + * If custom_cpu_runqueue[4] == -1
> > + * Then, it means that cpu 4 doesn't belong to any runqueue.
> > + */
> > + for ( i = 0; i < nr_cpu_ids; i++ )
> > + custom_cpu_runqueue[i] = -1;
> > +
> > + /*
> > + * Format [[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14,15]]
> > + */
> > + i = 0;
> > +
> > + /* In case user have spaces included in the input */
> > + if ( trim(s, m, ' ') != 0 )
> > + {
> > + printk( "WARNING : %s[%d] trim failed.\n", __func__,
> > __LINE__ );
> > + goto errReturn;
> > + }
> >
> Right. So, as I was saying above, can't we just start parsing the
> string --with something like a `char *p; while ( *p != '\0' ) {...}`--
> and skip the spaces when we find them?
>
> > + /* Starting to parse and get the cpu information on trimmed data
> > */
> > + _s = m;
> > + s_end = _s + strlen(_s);
> > +
> > + /* The start and should always be in format of '[..]' */
> > + if ( ( '[' == *_s ) && ( ']' == *(s_end-1)) )
> >
> But in the cover you said we also support:
>
> credit2_runqueue=0,1,4,5;2,3,6,7;8,9,12,13;10,11,14,15
>
> So, what I think we want here is something that checks that, if the
> first character is '[', then the last must be ']', or we already know
> the format is invalid, and we can stop.
>
> And it would also be better to write an if that checks whether the
> format is _invalid_, and stop the parsing if that is the case. That
> saves a level of indentation for all the code that follows.
>
Yes. Modifying the code in similar lines.
> > + {
> > + char *start = NULL, *end = NULL;
> > + int cpu_added_to_runqueue = 0;
> >
> It looks like this can be bool.
>
> > + _s++;
> > + while ( ( _s != NULL ) && ( _s < s_end ) )
> > + {
> > + char *token_sub_str = NULL;
> >
> We want a blank line between variable definition and actual code.
>
> > + start = strstr(_s, "[");
> > + end = strstr(_s, "]");
> > + /* Validation checks for '[' and ']' to properly parse
> > the entries
> > + */
> > + if ( ( !start && !end ) || ( ( end == ( s_end -1 ) ) &&
> > start ) )
> > + goto errReturn;
> >
> No camel case either, please. 'err' is probably just fine. :-)
>
> But, honestly, I can't quite tell what's going on here. Why strstr()?
> You can just check whether the first char of a set is '[' by looking at
> the current string parsing cursor. If it is, you can take a note about
> that somewhere, to remember to check for a ']' at some point.
>
>
strstr() is actually helping in getting the subsets, from where we will further
parse the cpu's which will belong to same runqueue.
> > + /* Check for last entry */
> > + else if ( !start && ( end == ( s_end -1 ) ) )
> > + goto nextSet;
> > +
> > + /* If we have [] as entry, move to nextSet */
> > + if ( getlen ( start, end ) < 1 )
> > + goto nextSet;
> >
> 'next' (or 'next_set', but I think 'next' is fine).
>
> > + /* Start to parse the actual value */
> > + start++;
> > + /*
> > + * find token within the subset
> > + */
> > + do
> > + {
> > + int token = 0;
> > + int len = 0;
> > + /* Get cpu ids separated by ',' within each set */
> > + token_sub_str = strpbrk(start, ",");
> > + if ( ( !token_sub_str && start < end ) ||
> > + ( token_sub_str > end && token_sub_str > start )
> > )
> > + len = getlen(start, end);
> > + else
> > + len = getlen(start, token_sub_str);
> > +
> > + if ( len <= 0 )
> > + continue;
> > + /* GETTOKEN will get return the parse and populate
> > the cpu in
> > + * token
> > + */
> > + GETTOKEN(token, len, start , end );
> > +
> > + if ( token >= nr_cpu_ids)
> > + goto errReturn;
> > + /* If not set already */
> > + if ( custom_cpu_runqueue[token] == -1 )
> > + {
> > + custom_cpu_runqueue[token] = i;
> > + cpu_added_to_runqueue = 1;
> > + }
> > + else
> > + goto errReturn;
> > +
> > + if ( !token_sub_str || token_sub_str > end )
> > + goto nextSet;
> > +
> > + start = ++token_sub_str;
> > + } while ( start < end );
> > +nextSet:
> > + if ( cpu_added_to_runqueue )
> > + {
> > + i++;
> > + cpu_added_to_runqueue = 0;
> > + }
> >
> >
> > + _s = ++end;
> > + }
> >
> Ok, so this looks more complex that it could be, to me. Mostly because
> you're doing your own token parsing.
>
> In my mind, parsing a string like the ones we want to accept, should
> look not too different from what's done inside init_boot_pages().
>
> Indeed our case looks a bit more complicated, but the general idea
> should be the same, i.e.:
> - scan the string and deal with what you find in there online, not
> beforehand;
> - use simple_strtoul()
Sure thing. Trying to use the exising APIs
>
> > @@ -661,6 +820,15 @@ cpu_to_runqueue(struct csched2_private *prv,
> > unsigned int cpu)
> > struct csched2_runqueue_data *rqd;
> > unsigned int rqi;
> >
> > + if ( opt_runqueue == OPT_RUNQUEUE_CUSTOM )
> > + {
> > + if ( custom_cpu_runqueue[cpu] != -1 )
> > + {
> > + BUG_ON(custom_cpu_runqueue[cpu] >= nr_cpu_ids);
> > + return custom_cpu_runqueue[cpu];
> > + }
> >
> Wait, and then what happens if a CPU was not in any set, and hence has
> it's custom_cpu_runqueue element == -1 ?
>
> I am ok with it not being added to any runqueue, but for that to be
> what really happens, I think we need to change code in init_pdata()
> too.
>
> It would be also good to print a warning, in such case. This is a
> custom mode, and the user may well be doing such a thing for a reason,
> but it doesn't harm trying make sure nothing this bad happened by
> mistake.
>
Yes, its better we show warning in case we receive -1 in init_pdata().
Working on the updated patch, will share once done with the basic unit test.
Thanks for your comments.
Regards,
~Praveen.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-14 18:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 19:28 [RFC PATCH v3 0/2] xen: credit2: flexible configuration for runqueues Praveen Kumar
2017-03-30 19:28 ` [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation Praveen Kumar
2017-03-31 7:18 ` Dario Faggioli
2017-03-30 19:28 ` [RFC PATCH v3 2/2] xen: credit2: provide custom option to create Praveen Kumar
2017-03-31 8:32 ` Dario Faggioli
2017-04-14 18:14 ` Praveen Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.