Messages in this thread | | | Date | Thu, 8 Sep 2016 10:56:36 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 08/33] x86/intel_rdt: Add Class of service management |
| |
On Thu, 8 Sep 2016, Fenghua Yu wrote: > @@ -0,0 +1,12 @@ > +#ifndef _RDT_H_ > +#define _RDT_H_
The standard guard is
_ASM_X86_RDT_H
> + > +#ifdef CONFIG_INTEL_RDT
What's the purpose of sticking a struct definition inside an ifdef?
> + > +struct clos_cbm_table { > + unsigned long cbm; > + unsigned int clos_refcnt;
Please proper align the struct members for readability sake
unsigned long cbm; unsigned int clos_refcnt;
Adding a kerneldoc comment above the struct explaining it would not hurting either.
> +/* > + * cctable maintains 1:1 mapping between CLOSid and cache bitmask. > + */ > +static struct clos_cbm_table *cctable; > +/* > + * closid availability bit map. > + */ > +unsigned long *closmap; > +static DEFINE_MUTEX(rdtgroup_mutex); > + > +static inline void closid_get(u32 closid) > +{ > + struct clos_cbm_table *cct = &cctable[closid]; > + > + lockdep_assert_held(&rdtgroup_mutex); > + > + cct->clos_refcnt++;
So the whole thing can be written with a single line at the single call site.
cc_table[closid].clos_refcnt++;
And that already has the lockdep_assert_held() check. While companies might pay based on line counts, kernel development asks for readable and sensible code.
> +} > + > +static int closid_alloc(u32 *closid) > +{ > + u32 maxid; > + u32 id;
Please put variables with the same type into one line. There is no value in wasting screen estate.
> + > + lockdep_assert_held(&rdtgroup_mutex); > + > + maxid = boot_cpu_data.x86_cache_max_closid; > + id = find_first_zero_bit(closmap, maxid); > + if (id == maxid) > + return -ENOSPC; > + > + set_bit(id, closmap); > + closid_get(id); > + *closid = id; > + > + return 0; > +} > + > +static inline void closid_free(u32 closid) > +{ > + clear_bit(closid, closmap); > + cctable[closid].cbm = 0; > +} > + > +static void closid_put(u32 closid) > +{ > + struct clos_cbm_table *cct = &cctable[closid]; > + > + lockdep_assert_held(&rdtgroup_mutex); > + if (WARN_ON(!cct->clos_refcnt)) > + return; > + > + if (!--cct->clos_refcnt) > + closid_free(closid); > +} > > static int __init intel_rdt_late_init(void) > { > struct cpuinfo_x86 *c = &boot_cpu_data; > + u32 maxid; > + int err = 0, size; > > if (!cpu_has(c, X86_FEATURE_CAT_L3)) > return -ENODEV; > > - pr_info("Intel cache allocation detected\n"); > + maxid = c->x86_cache_max_closid; > > - return 0; > + size = maxid * sizeof(struct clos_cbm_table); > + cctable = kzalloc(size, GFP_KERNEL); > + if (!cctable) { > + err = -ENOMEM; > + goto out_err;
I told you that before: Using a goto just to return err is pointless and silly. What the hell is wrong with
return -ENOMEM; ???
> + } > + > + size = BITS_TO_LONGS(maxid) * sizeof(long); > + closmap = kzalloc(size, GFP_KERNEL); > + if (!closmap) { > + kfree(cctable); > + err = -ENOMEM; > + goto out_err;
Groan.
> + } > + > + pr_info("Intel cache allocation enabled\n"); > +out_err:
| |