Date:   Sun, 9 Oct 2016 17:31:25 -0500
From:   Nilay Vaish <nilayvaish@...il.com>
To:     Fenghua Yu <fenghua.yu@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Stephane Eranian <eranian@...gle.com>,
        Borislav Petkov <bp@...e.de>,
        Dave Hansen <dave.hansen@...el.com>, Shaohua Li <shli@...com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v3 11/18] x86/intel_rdt: Add basic resctrl filesystem support

On 7 October 2016 at 21:45, Fenghua Yu <fenghua.yu@...el.com> wrote:
> From: Fenghua Yu <fenghua.yu@...el.com>
>
> diff --git a/arch/x86/include/asm/intel_rdt.h b/arch/x86/include/asm/intel_rdt.h
> index bad8dc7..f63815c 100644
> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -2,6 +2,23 @@
>  #define _ASM_X86_INTEL_RDT_H
>
>  /**
> + * struct rdtgroup - store rdtgroup's data in resctrl file system.
> + * @kn:                                kernfs node
> + * @rdtgroup_list:             linked list for all rdtgroups
> + * @closid:                    closid for this rdtgroup
> + */
> +struct rdtgroup {
> +       struct kernfs_node      *kn;
> +       struct list_head        rdtgroup_list;
> +       int                     closid;
> +};
> +
> +/* List of all resource groups */
> +extern struct list_head rdt_all_groups;
> +
> +int __init rdtgroup_init(void);
> +
> +/**
>   * struct rdt_resource - attributes of an RDT resource
>   * @enabled:                   Is this feature enabled on this machine
>   * @name:                      Name to use in "schemata" file
> @@ -39,6 +56,7 @@ struct rdt_resource {
>         for (r = rdt_resources_all; r->name; r++) \
>                 if (r->enabled)
>
> +#define IA32_L3_QOS_CFG                0xc81
>  #define IA32_L3_CBM_BASE       0xc90
>
>  /**
> @@ -72,7 +90,7 @@ extern struct mutex rdtgroup_mutex;
>
>  int __init rdtgroup_init(void);

This statement appears about twenty lines above as well.


> diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> new file mode 100644
> index 0000000..c99d3a0
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
> @@ -0,0 +1,236 @@
> +/*
> + * User interface for Resource Alloction in Resource Director Technology(RDT)
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Fenghua Yu <fenghua.yu@...el.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * More information about RDT be found in the Intel (R) x86 Architecture
> + * Software Developer Manual.
> + */
> +
> +#define pr_fmt(fmt)    KBUILD_MODNAME ": " fmt
> +
> +#include <linux/fs.h>
> +#include <linux/sysfs.h>
> +#include <linux/kernfs.h>
> +#include <linux/slab.h>
> +
> +#include <uapi/linux/magic.h>
> +
> +#include <asm/intel_rdt.h>
> +
> +DEFINE_STATIC_KEY_FALSE(rdt_enable_key);
> +struct kernfs_root *rdt_root;
> +struct rdtgroup rdtgroup_default;
> +LIST_HEAD(rdt_all_groups);
> +
> +static void l3_qos_cfg_update(void *arg)
> +{
> +       struct rdt_resource *r = arg;
> +
> +       wrmsrl(IA32_L3_QOS_CFG, r->cdp_enabled);
> +}
> +
> +static void set_l3_qos_cfg(struct rdt_resource *r)
> +{
> +       struct list_head *l;
> +       struct rdt_domain *d;
> +       struct cpumask cpu_mask;
> +
> +       cpumask_clear(&cpu_mask);
> +       list_for_each(l, &r->domains) {
> +               d = list_entry(l, struct rdt_domain, list);
> +               cpumask_set_cpu(cpumask_any(&d->cpu_mask), &cpu_mask);
> +       }
> +       smp_call_function_many(&cpu_mask, l3_qos_cfg_update, r, 1);
> +}
> +
> +static int parse_rdtgroupfs_options(char *data, struct rdt_resource *r)
> +{
> +       char *token, *o = data;
> +
> +       while ((token = strsep(&o, ",")) != NULL) {
> +               if (!*token)
> +                       return -EINVAL;
> +
> +               if (!strcmp(token, "cdp"))
> +                       if (r->enabled && r->cdp_capable)
> +                               r->cdp_enabled = true;
> +       }
> +
> +       return 0;
> +}
> +
> +static struct dentry *rdt_mount(struct file_system_type *fs_type,
> +                               int flags, const char *unused_dev_name,
> +                               void *data)
> +{
> +       struct dentry *dentry;
> +       int ret;
> +       bool new_sb;
> +       struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3];
> +
> +       mutex_lock(&rdtgroup_mutex);
> +       /*
> +        * resctrl file system can only be mounted once.
> +        */
> +       if (static_branch_unlikely(&rdt_enable_key)) {
> +               dentry = ERR_PTR(-EBUSY);
> +               goto out;
> +       }
> +
> +       r->cdp_enabled = false;
> +       ret = parse_rdtgroupfs_options(data, r);
> +       if (ret) {
> +               dentry = ERR_PTR(ret);
> +               goto out;
> +       }
> +       if (r->cdp_enabled)
> +               r->num_closid = r->max_closid / 2;
> +       else
> +               r->num_closid = r->max_closid;
> +
> +       /* Recompute rdt_max_closid because CDP may have changed things. */
> +       rdt_max_closid = 0;
> +       for_each_rdt_resource(r)
> +               rdt_max_closid = max(rdt_max_closid, r->num_closid);
> +       if (rdt_max_closid > 32)
> +               rdt_max_closid = 32;
> +
> +       dentry = kernfs_mount(fs_type, flags, rdt_root,
> +                             RDTGROUP_SUPER_MAGIC, &new_sb);
> +       if (IS_ERR(dentry))
> +               goto out;
> +       if (!new_sb) {
> +               dentry = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +       r = &rdt_resources_all[RDT_RESOURCE_L3];
> +       if (r->cdp_capable)
> +               set_l3_qos_cfg(r);
> +       static_branch_enable(&rdt_enable_key);
> +
> +out:
> +       mutex_unlock(&rdtgroup_mutex);
> +
> +       return dentry;
> +}

I am not too happy with the function above.  I would have expected
that the function above executes the common code and also makes calls
to per resource functions.  The way it has been written as now, it
seems to be mixing up things.

> +
> +static void reset_all_cbms(struct rdt_resource *r)
> +{
> +       struct list_head *l;
> +       struct rdt_domain *d;
> +       struct msr_param msr_param;
> +       struct cpumask cpu_mask;
> +       int i;
> +
> +       cpumask_clear(&cpu_mask);
> +       msr_param.res = r;
> +       msr_param.low = 0;
> +       msr_param.high = r->max_closid;
> +
> +       list_for_each(l, &r->domains) {
> +               d = list_entry(l, struct rdt_domain, list);
> +               cpumask_set_cpu(cpumask_any(&d->cpu_mask), &cpu_mask);
> +
> +               for (i = 0; i < r->max_closid; i++)
> +                       d->cbm[i] = r->max_cbm;
> +       }
> +       smp_call_function_many(&cpu_mask, rdt_cbm_update, &msr_param, 1);
> +}
> +
> +static void rdt_kill_sb(struct super_block *sb)
> +{
> +       struct rdt_resource *r;
> +
> +       mutex_lock(&rdtgroup_mutex);
> +
> +       /*Put everything back to default values. */
> +       for_each_rdt_resource(r)
> +               reset_all_cbms(r);
> +       r = &rdt_resources_all[RDT_RESOURCE_L3];
> +       if (r->cdp_capable) {
> +               r->cdp_enabled = 0;
> +               set_l3_qos_cfg(r);
> +       }
> +
> +       static_branch_disable(&rdt_enable_key);
> +       kernfs_kill_sb(sb);
> +       mutex_unlock(&rdtgroup_mutex);
> +}
> +
> +static struct file_system_type rdt_fs_type = {
> +       .name    = "resctrl",
> +       .mount   = rdt_mount,
> +       .kill_sb = rdt_kill_sb,
> +};
> +
> +static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
> +};
> +
> +static int __init rdtgroup_setup_root(void)
> +{
> +       int ret;
> +
> +       rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
> +                                     KERNFS_ROOT_CREATE_DEACTIVATED,
> +                                     &rdtgroup_default);
> +       if (IS_ERR(rdt_root))
> +               return PTR_ERR(rdt_root);
> +
> +       mutex_lock(&rdtgroup_mutex);
> +
> +       rdtgroup_default.closid = 0;
> +       list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
> +
> +       rdtgroup_default.kn = rdt_root->kn;
> +       kernfs_activate(rdtgroup_default.kn);
> +
> +       mutex_unlock(&rdtgroup_mutex);
> +
> +       return ret;

You don't set ret anywhere.


--
Nilay

Powered by blists - more mailing lists