Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Wed, Jun 13 2018, James Simmons wrote: >> > With the cleanup of the libcfs SMP handling all UMP handling >> > was removed. In the process now various NULL pointers and >> > empty fields are return in the UMP case which causes lustre >> > to crash hard. Restore the proper UMP handling so Lustre can >> > properly function. >> >> Can't we just get lustre to handle the NULL pointer? >> Is most cases, the pointer is accessed through an accessor function, and >> on !CONFIG_SMP, that can be a static inline that doesn't even look at >> the pointer. > > Lots of NULL pointer checks for a structure allocated at libcfs module > start and only cleaned up at libcfs removal is not a clean approach. > So I have thought about it and I have to ask why allocate a global > struct cfs_cpu_table. It could be made static and fill it in which would > avoid the whole NULL pointer issue. Plus for the UMP case why allocate > a new cfs_cpu_table with cfs_cpt_table_alloc() which is exactly like > the default UMP cfs_cpu_table. Instead we could just return the pointer > to the static default cfs_cpt_tab every time. We still have the NULL > ctb_cpumask field to deal with. Does that sound like a better solution > to you? Doug what do you think? I'm not convinced there will be lots of NULL pointer checks - maybe one or two. Most the accesses to the structure are inside helper functions which are empty inlines in the UP build. However I don't object to a static cfs_cpt_tab if that turns out to make some code simpler. I would want it to be clear up-front which code was simplified so that an informed decision could be made. Thanks, NeilBrown > >> I really think this is a step backwards. If you can identify specific >> problems caused by the current code, I'm sure we can fix them. >> >> > >> > Signed-off-by: James Simmons >> > Signed-off-by: Amir Shehata >> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734 >> >> This bug doesn't seem to mention this patch at all >> >> > Reviewed-on: http://review.whamcloud.com/18916 >> >> Nor does this review. > > Yeah its mutated so much from what is in the Intel tree. > I do believe it was the last patch to touch this. signature.asc Description: PGP signature
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Wed, Jun 13 2018, James Simmons wrote: >> > With the cleanup of the libcfs SMP handling all UMP handling >> > was removed. In the process now various NULL pointers and >> > empty fields are return in the UMP case which causes lustre >> > to crash hard. Restore the proper UMP handling so Lustre can >> > properly function. >> >> Can't we just get lustre to handle the NULL pointer? >> Is most cases, the pointer is accessed through an accessor function, and >> on !CONFIG_SMP, that can be a static inline that doesn't even look at >> the pointer. > > Lots of NULL pointer checks for a structure allocated at libcfs module > start and only cleaned up at libcfs removal is not a clean approach. > So I have thought about it and I have to ask why allocate a global > struct cfs_cpu_table. It could be made static and fill it in which would > avoid the whole NULL pointer issue. Plus for the UMP case why allocate > a new cfs_cpu_table with cfs_cpt_table_alloc() which is exactly like > the default UMP cfs_cpu_table. Instead we could just return the pointer > to the static default cfs_cpt_tab every time. We still have the NULL > ctb_cpumask field to deal with. Does that sound like a better solution > to you? Doug what do you think? I'm not convinced there will be lots of NULL pointer checks - maybe one or two. Most the accesses to the structure are inside helper functions which are empty inlines in the UP build. However I don't object to a static cfs_cpt_tab if that turns out to make some code simpler. I would want it to be clear up-front which code was simplified so that an informed decision could be made. Thanks, NeilBrown > >> I really think this is a step backwards. If you can identify specific >> problems caused by the current code, I'm sure we can fix them. >> >> > >> > Signed-off-by: James Simmons >> > Signed-off-by: Amir Shehata >> > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734 >> >> This bug doesn't seem to mention this patch at all >> >> > Reviewed-on: http://review.whamcloud.com/18916 >> >> Nor does this review. > > Yeah its mutated so much from what is in the Intel tree. > I do believe it was the last patch to touch this. signature.asc Description: PGP signature
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
> > With the cleanup of the libcfs SMP handling all UMP handling > > was removed. In the process now various NULL pointers and > > empty fields are return in the UMP case which causes lustre > > to crash hard. Restore the proper UMP handling so Lustre can > > properly function. > > Can't we just get lustre to handle the NULL pointer? > Is most cases, the pointer is accessed through an accessor function, and > on !CONFIG_SMP, that can be a static inline that doesn't even look at > the pointer. Lots of NULL pointer checks for a structure allocated at libcfs module start and only cleaned up at libcfs removal is not a clean approach. So I have thought about it and I have to ask why allocate a global struct cfs_cpu_table. It could be made static and fill it in which would avoid the whole NULL pointer issue. Plus for the UMP case why allocate a new cfs_cpu_table with cfs_cpt_table_alloc() which is exactly like the default UMP cfs_cpu_table. Instead we could just return the pointer to the static default cfs_cpt_tab every time. We still have the NULL ctb_cpumask field to deal with. Does that sound like a better solution to you? Doug what do you think? > I really think this is a step backwards. If you can identify specific > problems caused by the current code, I'm sure we can fix them. > > > > > Signed-off-by: James Simmons > > Signed-off-by: Amir Shehata > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734 > > This bug doesn't seem to mention this patch at all > > > Reviewed-on: http://review.whamcloud.com/18916 > > Nor does this review. Yeah its mutated so much from what is in the Intel tree. I do believe it was the last patch to touch this.
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
> > With the cleanup of the libcfs SMP handling all UMP handling > > was removed. In the process now various NULL pointers and > > empty fields are return in the UMP case which causes lustre > > to crash hard. Restore the proper UMP handling so Lustre can > > properly function. > > Can't we just get lustre to handle the NULL pointer? > Is most cases, the pointer is accessed through an accessor function, and > on !CONFIG_SMP, that can be a static inline that doesn't even look at > the pointer. Lots of NULL pointer checks for a structure allocated at libcfs module start and only cleaned up at libcfs removal is not a clean approach. So I have thought about it and I have to ask why allocate a global struct cfs_cpu_table. It could be made static and fill it in which would avoid the whole NULL pointer issue. Plus for the UMP case why allocate a new cfs_cpu_table with cfs_cpt_table_alloc() which is exactly like the default UMP cfs_cpu_table. Instead we could just return the pointer to the static default cfs_cpt_tab every time. We still have the NULL ctb_cpumask field to deal with. Does that sound like a better solution to you? Doug what do you think? > I really think this is a step backwards. If you can identify specific > problems caused by the current code, I'm sure we can fix them. > > > > > Signed-off-by: James Simmons > > Signed-off-by: Amir Shehata > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734 > > This bug doesn't seem to mention this patch at all > > > Reviewed-on: http://review.whamcloud.com/18916 > > Nor does this review. Yeah its mutated so much from what is in the Intel tree. I do believe it was the last patch to touch this.
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Tue, May 29, 2018 at 10:21:41AM -0400, James Simmons wrote:
> @@ -208,20 +222,52 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> void cfs_cpu_fini(void);
>
> #else /* !CONFIG_SMP */
> -struct cfs_cpt_table;
> -#define cfs_cpt_tab ((struct cfs_cpt_table *)NULL)
>
> -static inline cpumask_var_t *
> -cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +static inline void cfs_cpt_table_free(struct cfs_cpt_table *cptab)
> {
> - return NULL;
> + kfree(cptab);
> }
This should free cptab->ctb_cpumask?
regards,
dan carpenter
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Tue, May 29, 2018 at 10:21:41AM -0400, James Simmons wrote:
> @@ -208,20 +222,52 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> void cfs_cpu_fini(void);
>
> #else /* !CONFIG_SMP */
> -struct cfs_cpt_table;
> -#define cfs_cpt_tab ((struct cfs_cpt_table *)NULL)
>
> -static inline cpumask_var_t *
> -cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +static inline void cfs_cpt_table_free(struct cfs_cpt_table *cptab)
> {
> - return NULL;
> + kfree(cptab);
> }
This should free cptab->ctb_cpumask?
regards,
dan carpenter
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Tue, May 29 2018, James Simmons wrote:
> With the cleanup of the libcfs SMP handling all UMP handling
> was removed. In the process now various NULL pointers and
> empty fields are return in the UMP case which causes lustre
> to crash hard. Restore the proper UMP handling so Lustre can
> properly function.
Can't we just get lustre to handle the NULL pointer?
Is most cases, the pointer is accessed through an accessor function, and
on !CONFIG_SMP, that can be a static inline that doesn't even look at
the pointer.
I really think this is a step backwards. If you can identify specific
problems caused by the current code, I'm sure we can fix them.
>
> Signed-off-by: James Simmons
> Signed-off-by: Amir Shehata
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734
This bug doesn't seem to mention this patch at all
> Reviewed-on: http://review.whamcloud.com/18916
Nor does this review.
Thanks,
NeilBrown
> Reviewed-by: Olaf Weber
> Reviewed-by: Doug Oucharek
> Signed-off-by: James Simmons
> ---
> Changelog:
>
> v1) New patch to handle the disappearence of UMP support
>
> .../lustre/include/linux/libcfs/libcfs_cpu.h | 87
> --
> drivers/staging/lustre/lnet/libcfs/libcfs_cpu.c| 4 -
> drivers/staging/lustre/lnet/libcfs/module.c| 4 +
> 3 files changed, 69 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> b/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> index 61641c4..2ad12a6 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> @@ -74,6 +74,7 @@
>
> #include
> #include
> +#include
> #include
>
> /* any CPU partition */
> @@ -89,10 +90,11 @@ struct cfs_cpu_partition {
> /* spread rotor for NUMA allocator */
> unsigned intcpt_spread_rotor;
> };
> -
> +#endif /* CONFIG_SMP */
>
> /** descriptor for CPU partitions */
> struct cfs_cpt_table {
> +#ifdef CONFIG_SMP
> /* version, reserved for hotplug */
> unsigned intctb_version;
> /* spread rotor for NUMA allocator */
> @@ -103,14 +105,26 @@ struct cfs_cpt_table {
> struct cfs_cpu_partition*ctb_parts;
> /* shadow HW CPU to CPU partition ID */
> int *ctb_cpu2cpt;
> - /* all cpus in this partition table */
> - cpumask_var_t ctb_cpumask;
> /* all nodes in this partition table */
> nodemask_t *ctb_nodemask;
> +#else
> + nodemask_t ctb_nodemask;
> +#endif /* CONFIG_SMP */
> + /* all cpus in this partition table */
> + cpumask_var_t ctb_cpumask;
> };
>
> extern struct cfs_cpt_table *cfs_cpt_tab;
>
> +#ifdef CONFIG_SMP
> +/**
> + * destroy a CPU partition table
> + */
> +void cfs_cpt_table_free(struct cfs_cpt_table *cptab);
> +/**
> + * create a cfs_cpt_table with \a ncpt number of partitions
> + */
> +struct cfs_cpt_table *cfs_cpt_table_alloc(unsigned int ncpt);
> /**
> * return cpumask of CPU partition \a cpt
> */
> @@ -208,20 +222,52 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> void cfs_cpu_fini(void);
>
> #else /* !CONFIG_SMP */
> -struct cfs_cpt_table;
> -#define cfs_cpt_tab ((struct cfs_cpt_table *)NULL)
>
> -static inline cpumask_var_t *
> -cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +static inline void cfs_cpt_table_free(struct cfs_cpt_table *cptab)
> {
> - return NULL;
> + kfree(cptab);
> }
>
> -static inline int
> -cfs_cpt_table_print(struct cfs_cpt_table *cptab, char *buf, int len)
> +static inline struct cfs_cpt_table *cfs_cpt_table_alloc(int ncpt)
> {
> - return 0;
> + struct cfs_cpt_table *cptab;
> +
> + if (ncpt != 1)
> + return NULL;
> +
> + cptab = kzalloc(sizeof(*cptab), GFP_NOFS);
> + if (!cptab)
> + return NULL;
> +
> + if (!zalloc_cpumask_var(>ctb_cpumask, GFP_NOFS)) {
> + kfree(cptab);
> + return NULL;
> + }
> + cpumask_set_cpu(0, cptab->ctb_cpumask);
> + node_set(0, cptab->ctb_nodemask);
> +
> + return cptab;
> +}
> +
> +static inline int cfs_cpt_table_print(struct cfs_cpt_table *cptab,
> + char *buf, int len)
> +{
> + int rc;
> +
> + rc = snprintf(buf, len, "0\t: 0\n");
> + len -= rc;
> + if (len <= 0)
> + return -EFBIG;
> +
> + return rc;
> }
> +
> +static inline cpumask_var_t *
> +cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +{
> + return >ctb_cpumask;
> +}
> +
> static inline int
> cfs_cpt_number(struct cfs_cpt_table *cptab)
> {
> @@ -243,7 +289,7 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> static inline nodemask_t *
> cfs_cpt_nodemask(struct cfs_cpt_table *cptab, int cpt)
> {
> - return NULL;
> + return >ctb_nodemask;
> }
>
>
Re: [PATCH v2 01/25] staging: lustre: libcfs: restore UMP handling
On Tue, May 29 2018, James Simmons wrote:
> With the cleanup of the libcfs SMP handling all UMP handling
> was removed. In the process now various NULL pointers and
> empty fields are return in the UMP case which causes lustre
> to crash hard. Restore the proper UMP handling so Lustre can
> properly function.
Can't we just get lustre to handle the NULL pointer?
Is most cases, the pointer is accessed through an accessor function, and
on !CONFIG_SMP, that can be a static inline that doesn't even look at
the pointer.
I really think this is a step backwards. If you can identify specific
problems caused by the current code, I'm sure we can fix them.
>
> Signed-off-by: James Simmons
> Signed-off-by: Amir Shehata
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-7734
This bug doesn't seem to mention this patch at all
> Reviewed-on: http://review.whamcloud.com/18916
Nor does this review.
Thanks,
NeilBrown
> Reviewed-by: Olaf Weber
> Reviewed-by: Doug Oucharek
> Signed-off-by: James Simmons
> ---
> Changelog:
>
> v1) New patch to handle the disappearence of UMP support
>
> .../lustre/include/linux/libcfs/libcfs_cpu.h | 87
> --
> drivers/staging/lustre/lnet/libcfs/libcfs_cpu.c| 4 -
> drivers/staging/lustre/lnet/libcfs/module.c| 4 +
> 3 files changed, 69 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> b/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> index 61641c4..2ad12a6 100644
> --- a/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> +++ b/drivers/staging/lustre/include/linux/libcfs/libcfs_cpu.h
> @@ -74,6 +74,7 @@
>
> #include
> #include
> +#include
> #include
>
> /* any CPU partition */
> @@ -89,10 +90,11 @@ struct cfs_cpu_partition {
> /* spread rotor for NUMA allocator */
> unsigned intcpt_spread_rotor;
> };
> -
> +#endif /* CONFIG_SMP */
>
> /** descriptor for CPU partitions */
> struct cfs_cpt_table {
> +#ifdef CONFIG_SMP
> /* version, reserved for hotplug */
> unsigned intctb_version;
> /* spread rotor for NUMA allocator */
> @@ -103,14 +105,26 @@ struct cfs_cpt_table {
> struct cfs_cpu_partition*ctb_parts;
> /* shadow HW CPU to CPU partition ID */
> int *ctb_cpu2cpt;
> - /* all cpus in this partition table */
> - cpumask_var_t ctb_cpumask;
> /* all nodes in this partition table */
> nodemask_t *ctb_nodemask;
> +#else
> + nodemask_t ctb_nodemask;
> +#endif /* CONFIG_SMP */
> + /* all cpus in this partition table */
> + cpumask_var_t ctb_cpumask;
> };
>
> extern struct cfs_cpt_table *cfs_cpt_tab;
>
> +#ifdef CONFIG_SMP
> +/**
> + * destroy a CPU partition table
> + */
> +void cfs_cpt_table_free(struct cfs_cpt_table *cptab);
> +/**
> + * create a cfs_cpt_table with \a ncpt number of partitions
> + */
> +struct cfs_cpt_table *cfs_cpt_table_alloc(unsigned int ncpt);
> /**
> * return cpumask of CPU partition \a cpt
> */
> @@ -208,20 +222,52 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> void cfs_cpu_fini(void);
>
> #else /* !CONFIG_SMP */
> -struct cfs_cpt_table;
> -#define cfs_cpt_tab ((struct cfs_cpt_table *)NULL)
>
> -static inline cpumask_var_t *
> -cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +static inline void cfs_cpt_table_free(struct cfs_cpt_table *cptab)
> {
> - return NULL;
> + kfree(cptab);
> }
>
> -static inline int
> -cfs_cpt_table_print(struct cfs_cpt_table *cptab, char *buf, int len)
> +static inline struct cfs_cpt_table *cfs_cpt_table_alloc(int ncpt)
> {
> - return 0;
> + struct cfs_cpt_table *cptab;
> +
> + if (ncpt != 1)
> + return NULL;
> +
> + cptab = kzalloc(sizeof(*cptab), GFP_NOFS);
> + if (!cptab)
> + return NULL;
> +
> + if (!zalloc_cpumask_var(>ctb_cpumask, GFP_NOFS)) {
> + kfree(cptab);
> + return NULL;
> + }
> + cpumask_set_cpu(0, cptab->ctb_cpumask);
> + node_set(0, cptab->ctb_nodemask);
> +
> + return cptab;
> +}
> +
> +static inline int cfs_cpt_table_print(struct cfs_cpt_table *cptab,
> + char *buf, int len)
> +{
> + int rc;
> +
> + rc = snprintf(buf, len, "0\t: 0\n");
> + len -= rc;
> + if (len <= 0)
> + return -EFBIG;
> +
> + return rc;
> }
> +
> +static inline cpumask_var_t *
> +cfs_cpt_cpumask(struct cfs_cpt_table *cptab, int cpt)
> +{
> + return >ctb_cpumask;
> +}
> +
> static inline int
> cfs_cpt_number(struct cfs_cpt_table *cptab)
> {
> @@ -243,7 +289,7 @@ void cfs_cpt_unset_nodemask(struct cfs_cpt_table *cptab,
> static inline nodemask_t *
> cfs_cpt_nodemask(struct cfs_cpt_table *cptab, int cpt)
> {
> - return NULL;
> + return >ctb_nodemask;
> }
>
>
