On 09/08/2016 02:57 AM, Fenghua Yu wrote:
> +static int __init rdt_setup(char *str)
> +{
> + char *tok;
> +
> + while ((tok = strsep(&str, ",")) != NULL) {
> + if (!*tok)
> + return -EINVAL;
> +
> + if (strcmp(tok, "simulate_cat_l3") == 0) {
> + pr_info("Simulate CAT L3\n");
> + rdt_opts.simulate_cat_l3 = true;
> + } else if (strcmp(tok, "disable_cat_l3") == 0) {
> + pr_info("CAT L3 is disabled\n");
> + disable_cat_l3 = true;
> + } else {
> + pr_info("Invalid rdt option\n");
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +__setup("resctrl=", rdt_setup);
So, this allows you to specify both simulation and disabling at the same
time, and in the same option? That seems a bit screwy, plus it requires
some parsing which is quite prone to being broken. How about just
having two setup options:
__setup("resctrl=simulate_cat_l3", rdt_setup...);
__setup("resctrl=disable_cat_l3", rdt_setup...);
And allow folks to specify "resctrl" more than once instead of requiring
the comma-separated arguments? Then you don't have to do any parsing at
all and your __setup() handlers become one-liners.
Is "resctrl" really the best name for this sucker? Wouldn't
"intel-rdt=" or something be nicer?
Also, a lot of __setup() functions actually clear cpuid bits. Should
this be clearing X86_FEATURE_CAT_L3 instead of keeping a boolean around
that effectively overrides it?
> +static inline bool cat_l3_supported(struct cpuinfo_x86 *c)
> +{
> + if (cpu_has(c, X86_FEATURE_CAT_L3))
> + return true;
> +
> + /*
> + * Probe for Haswell server CPUs.
> + */
> + if (c->x86 == 0x6 && c->x86_model == 0x3f)
> + return cache_alloc_hsw_probe();
> +
> + return false;
> +}
#include <asm/intel-family.h> and s/0x3f/INTEL_FAM6_HASWELL_X/, please.
Then your comment even go away.
