2017-12-18 23:02:04

by NeilBrown

[permalink] [raw]
Subject: [PATCH SET 6: 0/2] staging: lustre: two miscellaneous patches

I thought I had mode one-off patches but this, but
it seems there are only two ready to go now.
The first removes another local interface in favour of a more general
interface, the second removing a warning (which actually say "BUG:").

Thanks,
NeilBrown


---

NeilBrown (2):
staging: lustre: use strim instead of cfs_trimwhite.
staging: lustre: disable preempt while sampling processor id.


.../lustre/include/linux/libcfs/libcfs_string.h | 1 -
drivers/staging/lustre/lnet/libcfs/libcfs_string.c | 20 -------------------
.../staging/lustre/lnet/libcfs/linux/linux-cpu.c | 21 ++++++++++----------
drivers/staging/lustre/lnet/lnet/config.c | 10 +++++-----
drivers/staging/lustre/lnet/lnet/router_proc.c | 2 +-
5 files changed, 17 insertions(+), 37 deletions(-)

--
Signature


2017-12-18 23:02:18

by NeilBrown

[permalink] [raw]
Subject: [PATCH 1/2] staging: lustre: use strim instead of cfs_trimwhite.

Linux lib provides identical functionality to cfs_trimwhite,
so discard that code and use the standard.

Signed-off-by: NeilBrown <[email protected]>
---
.../lustre/include/linux/libcfs/libcfs_string.h | 1 -
drivers/staging/lustre/lnet/libcfs/libcfs_string.c | 20 --------------------
.../staging/lustre/lnet/libcfs/linux/linux-cpu.c | 8 ++++----
drivers/staging/lustre/lnet/lnet/config.c | 10 +++++-----
drivers/staging/lustre/lnet/lnet/router_proc.c | 2 +-
5 files changed, 10 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
index c1375733ff31..66463477074a 100644
--- a/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
+++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
@@ -73,7 +73,6 @@ struct cfs_expr_list {
struct list_head el_exprs;
};

-char *cfs_trimwhite(char *str);
int cfs_gettok(struct cfs_lstr *next, char delim, struct cfs_lstr *res);
int cfs_str2num_check(char *str, int nob, unsigned int *num,
unsigned int min, unsigned int max);
diff --git a/drivers/staging/lustre/lnet/libcfs/libcfs_string.c b/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
index b1d8faa3f7aa..442889a3d729 100644
--- a/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
+++ b/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
@@ -137,26 +137,6 @@ char *cfs_firststr(char *str, size_t size)
}
EXPORT_SYMBOL(cfs_firststr);

-char *
-cfs_trimwhite(char *str)
-{
- char *end;
-
- while (isspace(*str))
- str++;
-
- end = str + strlen(str);
- while (end > str) {
- if (!isspace(end[-1]))
- break;
- end--;
- }
-
- *end = 0;
- return str;
-}
-EXPORT_SYMBOL(cfs_trimwhite);
-
/**
* Extracts tokens from strings.
*
diff --git a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
index e9156bf05ed4..d30650f8dcb4 100644
--- a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
+++ b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
@@ -818,7 +818,7 @@ cfs_cpt_table_create_pattern(char *pattern)
int c;
int i;

- str = cfs_trimwhite(pattern);
+ str = strim(pattern);
if (*str == 'n' || *str == 'N') {
pattern = str + 1;
if (*pattern != '\0') {
@@ -870,7 +870,7 @@ cfs_cpt_table_create_pattern(char *pattern)

high = node ? MAX_NUMNODES - 1 : nr_cpu_ids - 1;

- for (str = cfs_trimwhite(pattern), c = 0;; c++) {
+ for (str = strim(pattern), c = 0;; c++) {
struct cfs_range_expr *range;
struct cfs_expr_list *el;
char *bracket = strchr(str, '[');
@@ -905,7 +905,7 @@ cfs_cpt_table_create_pattern(char *pattern)
goto failed;
}

- str = cfs_trimwhite(str + n);
+ str = strim(str + n);
if (str != bracket) {
CERROR("Invalid pattern %s\n", str);
goto failed;
@@ -945,7 +945,7 @@ cfs_cpt_table_create_pattern(char *pattern)
goto failed;
}

- str = cfs_trimwhite(bracket + 1);
+ str = strim(bracket + 1);
}

return cptab;
diff --git a/drivers/staging/lustre/lnet/lnet/config.c b/drivers/staging/lustre/lnet/lnet/config.c
index fd53c74766a7..44eeca63f458 100644
--- a/drivers/staging/lustre/lnet/lnet/config.c
+++ b/drivers/staging/lustre/lnet/lnet/config.c
@@ -269,7 +269,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)

if (comma)
*comma++ = 0;
- net = libcfs_str2net(cfs_trimwhite(str));
+ net = libcfs_str2net(strim(str));

if (net == LNET_NIDNET(LNET_NID_ANY)) {
LCONSOLE_ERROR_MSG(0x113,
@@ -292,7 +292,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
}

*bracket = 0;
- net = libcfs_str2net(cfs_trimwhite(str));
+ net = libcfs_str2net(strim(str));
if (net == LNET_NIDNET(LNET_NID_ANY)) {
tmp = str;
goto failed_syntax;
@@ -322,7 +322,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
if (comma)
*comma++ = 0;

- iface = cfs_trimwhite(iface);
+ iface = strim(iface);
if (!*iface) {
tmp = iface;
goto failed_syntax;
@@ -356,7 +356,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
comma = strchr(bracket + 1, ',');
if (comma) {
*comma = 0;
- str = cfs_trimwhite(str);
+ str = strim(str);
if (*str) {
tmp = str;
goto failed_syntax;
@@ -365,7 +365,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
continue;
}

- str = cfs_trimwhite(str);
+ str = strim(str);
if (*str) {
tmp = str;
goto failed_syntax;
diff --git a/drivers/staging/lustre/lnet/lnet/router_proc.c b/drivers/staging/lustre/lnet/lnet/router_proc.c
index 8d6d6b4d6619..1a71ffebc889 100644
--- a/drivers/staging/lustre/lnet/lnet/router_proc.c
+++ b/drivers/staging/lustre/lnet/lnet/router_proc.c
@@ -829,7 +829,7 @@ static int __proc_lnet_portal_rotor(void *data, int write,
if (rc < 0)
goto out;

- tmp = cfs_trimwhite(buf);
+ tmp = strim(buf);

rc = -EINVAL;
lnet_res_lock(0);


2017-12-18 23:02:23

by NeilBrown

[permalink] [raw]
Subject: [PATCH 2/2] staging: lustre: disable preempt while sampling processor id.

Calling smp_processor_id() without disabling preemption
triggers a warning (if CONFIG_DEBUG_PREEMPT).
I think the result of cfs_cpt_current() is only used as a hint for
load balancing, rather than as a precise and stable indicator of
the current CPU. So it doesn't need to be called with
preemption disabled.

So disable preemption inside cfs_cpt_current() to silence the warning.

Signed-off-by: NeilBrown <[email protected]>
---
.../staging/lustre/lnet/libcfs/linux/linux-cpu.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
index d30650f8dcb4..ca8518b8a3e0 100644
--- a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
+++ b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
@@ -517,19 +517,20 @@ EXPORT_SYMBOL(cfs_cpt_spread_node);
int
cfs_cpt_current(struct cfs_cpt_table *cptab, int remap)
{
- int cpu = smp_processor_id();
- int cpt = cptab->ctb_cpu2cpt[cpu];
+ int cpu;
+ int cpt;

- if (cpt < 0) {
- if (!remap)
- return cpt;
+ preempt_disable();
+ cpu = smp_processor_id();
+ cpt = cptab->ctb_cpu2cpt[cpu];

+ if (cpt < 0 && remap) {
/* don't return negative value for safety of upper layer,
* instead we shadow the unknown cpu to a valid partition ID
*/
cpt = cpu % cptab->ctb_nparts;
}
-
+ preempt_enable();
return cpt;
}
EXPORT_SYMBOL(cfs_cpt_current);


2017-12-19 23:37:34

by Dilger, Andreas

[permalink] [raw]
Subject: Re: [PATCH 2/2] staging: lustre: disable preempt while sampling processor id.

On Dec 18, 2017, at 16:01, NeilBrown <[email protected]> wrote:
>
> Calling smp_processor_id() without disabling preemption
> triggers a warning (if CONFIG_DEBUG_PREEMPT).
> I think the result of cfs_cpt_current() is only used as a hint for
> load balancing, rather than as a precise and stable indicator of
> the current CPU. So it doesn't need to be called with
> preemption disabled.
>
> So disable preemption inside cfs_cpt_current() to silence the warning.
>
> Signed-off-by: NeilBrown <[email protected]>

Reviewed-by: Andreas Dilger <[email protected]>

> ---
> .../staging/lustre/lnet/libcfs/linux/linux-cpu.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> index d30650f8dcb4..ca8518b8a3e0 100644
> --- a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> +++ b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> @@ -517,19 +517,20 @@ EXPORT_SYMBOL(cfs_cpt_spread_node);
> int
> cfs_cpt_current(struct cfs_cpt_table *cptab, int remap)
> {
> - int cpu = smp_processor_id();
> - int cpt = cptab->ctb_cpu2cpt[cpu];
> + int cpu;
> + int cpt;
>
> - if (cpt < 0) {
> - if (!remap)
> - return cpt;
> + preempt_disable();
> + cpu = smp_processor_id();
> + cpt = cptab->ctb_cpu2cpt[cpu];
>
> + if (cpt < 0 && remap) {
> /* don't return negative value for safety of upper layer,
> * instead we shadow the unknown cpu to a valid partition ID
> */
> cpt = cpu % cptab->ctb_nparts;
> }
> -
> + preempt_enable();
> return cpt;
> }
> EXPORT_SYMBOL(cfs_cpt_current);
>
>

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation







2017-12-19 23:38:11

by Dilger, Andreas

[permalink] [raw]
Subject: Re: [PATCH 1/2] staging: lustre: use strim instead of cfs_trimwhite.

On Dec 18, 2017, at 16:01, NeilBrown <[email protected]> wrote:
>
> Linux lib provides identical functionality to cfs_trimwhite,
> so discard that code and use the standard.
>
> Signed-off-by: NeilBrown <[email protected]>

Reviewed-by: Andreas Dilger <[email protected]>

> ---
> .../lustre/include/linux/libcfs/libcfs_string.h | 1 -
> drivers/staging/lustre/lnet/libcfs/libcfs_string.c | 20 --------------------
> .../staging/lustre/lnet/libcfs/linux/linux-cpu.c | 8 ++++----
> drivers/staging/lustre/lnet/lnet/config.c | 10 +++++-----
> drivers/staging/lustre/lnet/lnet/router_proc.c | 2 +-
> 5 files changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h b/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
> index c1375733ff31..66463477074a 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_string.h
> @@ -73,7 +73,6 @@ struct cfs_expr_list {
> struct list_head el_exprs;
> };
>
> -char *cfs_trimwhite(char *str);
> int cfs_gettok(struct cfs_lstr *next, char delim, struct cfs_lstr *res);
> int cfs_str2num_check(char *str, int nob, unsigned int *num,
> unsigned int min, unsigned int max);
> diff --git a/drivers/staging/lustre/lnet/libcfs/libcfs_string.c b/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
> index b1d8faa3f7aa..442889a3d729 100644
> --- a/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
> +++ b/drivers/staging/lustre/lnet/libcfs/libcfs_string.c
> @@ -137,26 +137,6 @@ char *cfs_firststr(char *str, size_t size)
> }
> EXPORT_SYMBOL(cfs_firststr);
>
> -char *
> -cfs_trimwhite(char *str)
> -{
> - char *end;
> -
> - while (isspace(*str))
> - str++;
> -
> - end = str + strlen(str);
> - while (end > str) {
> - if (!isspace(end[-1]))
> - break;
> - end--;
> - }
> -
> - *end = 0;
> - return str;
> -}
> -EXPORT_SYMBOL(cfs_trimwhite);
> -
> /**
> * Extracts tokens from strings.
> *
> diff --git a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> index e9156bf05ed4..d30650f8dcb4 100644
> --- a/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> +++ b/drivers/staging/lustre/lnet/libcfs/linux/linux-cpu.c
> @@ -818,7 +818,7 @@ cfs_cpt_table_create_pattern(char *pattern)
> int c;
> int i;
>
> - str = cfs_trimwhite(pattern);
> + str = strim(pattern);
> if (*str == 'n' || *str == 'N') {
> pattern = str + 1;
> if (*pattern != '\0') {
> @@ -870,7 +870,7 @@ cfs_cpt_table_create_pattern(char *pattern)
>
> high = node ? MAX_NUMNODES - 1 : nr_cpu_ids - 1;
>
> - for (str = cfs_trimwhite(pattern), c = 0;; c++) {
> + for (str = strim(pattern), c = 0;; c++) {
> struct cfs_range_expr *range;
> struct cfs_expr_list *el;
> char *bracket = strchr(str, '[');
> @@ -905,7 +905,7 @@ cfs_cpt_table_create_pattern(char *pattern)
> goto failed;
> }
>
> - str = cfs_trimwhite(str + n);
> + str = strim(str + n);
> if (str != bracket) {
> CERROR("Invalid pattern %s\n", str);
> goto failed;
> @@ -945,7 +945,7 @@ cfs_cpt_table_create_pattern(char *pattern)
> goto failed;
> }
>
> - str = cfs_trimwhite(bracket + 1);
> + str = strim(bracket + 1);
> }
>
> return cptab;
> diff --git a/drivers/staging/lustre/lnet/lnet/config.c b/drivers/staging/lustre/lnet/lnet/config.c
> index fd53c74766a7..44eeca63f458 100644
> --- a/drivers/staging/lustre/lnet/lnet/config.c
> +++ b/drivers/staging/lustre/lnet/lnet/config.c
> @@ -269,7 +269,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
>
> if (comma)
> *comma++ = 0;
> - net = libcfs_str2net(cfs_trimwhite(str));
> + net = libcfs_str2net(strim(str));
>
> if (net == LNET_NIDNET(LNET_NID_ANY)) {
> LCONSOLE_ERROR_MSG(0x113,
> @@ -292,7 +292,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
> }
>
> *bracket = 0;
> - net = libcfs_str2net(cfs_trimwhite(str));
> + net = libcfs_str2net(strim(str));
> if (net == LNET_NIDNET(LNET_NID_ANY)) {
> tmp = str;
> goto failed_syntax;
> @@ -322,7 +322,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
> if (comma)
> *comma++ = 0;
>
> - iface = cfs_trimwhite(iface);
> + iface = strim(iface);
> if (!*iface) {
> tmp = iface;
> goto failed_syntax;
> @@ -356,7 +356,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
> comma = strchr(bracket + 1, ',');
> if (comma) {
> *comma = 0;
> - str = cfs_trimwhite(str);
> + str = strim(str);
> if (*str) {
> tmp = str;
> goto failed_syntax;
> @@ -365,7 +365,7 @@ lnet_parse_networks(struct list_head *nilist, char *networks)
> continue;
> }
>
> - str = cfs_trimwhite(str);
> + str = strim(str);
> if (*str) {
> tmp = str;
> goto failed_syntax;
> diff --git a/drivers/staging/lustre/lnet/lnet/router_proc.c b/drivers/staging/lustre/lnet/lnet/router_proc.c
> index 8d6d6b4d6619..1a71ffebc889 100644
> --- a/drivers/staging/lustre/lnet/lnet/router_proc.c
> +++ b/drivers/staging/lustre/lnet/lnet/router_proc.c
> @@ -829,7 +829,7 @@ static int __proc_lnet_portal_rotor(void *data, int write,
> if (rc < 0)
> goto out;
>
> - tmp = cfs_trimwhite(buf);
> + tmp = strim(buf);
>
> rc = -EINVAL;
> lnet_res_lock(0);
>
>

Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation