Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Wed, Mar 24, 2021 at 09:17:01AM -0700, Jacob Pan wrote: > I didn't mean the users of misc_cgroup will use css directly. I meant if I > want to use misc cgruop in ioasid.c, I have to do the following to avoid > undefined css: > #include > #include > > So it might be simpler if you do #include inside > misc_cgroup.h. Then in ioasid.c, I only need to do > #include . Sorry, I misunderstood the comment first time. I agree with you, I will add cgroup header file in the misc_cgroup.h after #ifdef CONFIG_CGROUP_MISC statement. Thanks Vipin
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
Hi Vipin,
On Mon, 22 Mar 2021 11:54:39 -0700, Vipin Sharma wrote:
> On Fri, Mar 19, 2021 at 02:28:01PM -0700, Jacob Pan wrote:
> > On Thu, 4 Mar 2021 15:19:45 -0800, Vipin Sharma
> > wrote:
> > > +#ifndef _MISC_CGROUP_H_
> > > +#define _MISC_CGROUP_H_
> > > +
> > nit: should you do #include ?
> > Otherwise, css may be undefined.
>
> User of this controller will use get_curernt_misc_cg() API which returns
> a pointer. Ideally the user should use this pointer and they shouldn't
> have any need to access "css" in their code. They also don't need to
> create a object of 'struct misc_cg{}', because that won't be correct misc
> cgroup object. They should just declare a pointer like we are doing here
> in 'struct kvm_sev_info {}'.
>
> If they do need to use "css" then they can include cgroup header in their
> code.
>
I didn't mean the users of misc_cgroup will use css directly. I meant if I
want to use misc cgruop in ioasid.c, I have to do the following to avoid
undefined css:
#include
#include
So it might be simpler if you do #include inside
misc_cgroup.h. Then in ioasid.c, I only need to do
#include .
> Let me know if I am overlooking something here.
>
> Thanks
> Vipin Sharma
Thanks,
Jacob
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Fri, Mar 19, 2021 at 02:28:01PM -0700, Jacob Pan wrote:
> On Thu, 4 Mar 2021 15:19:45 -0800, Vipin Sharma wrote:
> > +#ifndef _MISC_CGROUP_H_
> > +#define _MISC_CGROUP_H_
> > +
> nit: should you do #include ?
> Otherwise, css may be undefined.
User of this controller will use get_curernt_misc_cg() API which returns
a pointer. Ideally the user should use this pointer and they shouldn't have
any need to access "css" in their code. They also don't need to create a
object of 'struct misc_cg{}', because that won't be correct misc cgroup
object. They should just declare a pointer like we are doing here in
'struct kvm_sev_info {}'.
If they do need to use "css" then they can include cgroup header in their
code.
Let me know if I am overlooking something here.
Thanks
Vipin Sharma
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
Hi Vipin, On Thu, 4 Mar 2021 15:19:45 -0800, Vipin Sharma wrote: > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Miscellaneous cgroup controller. > + * > + * Copyright 2020 Google LLC > + * Author: Vipin Sharma > + */ > +#ifndef _MISC_CGROUP_H_ > +#define _MISC_CGROUP_H_ > + nit: should you do #include ? Otherwise, css may be undefined. > +/** > + * Types of misc cgroup entries supported by the host. > + */ Thanks, Jacob
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Fri, Mar 12, 2021 at 11:07:14AM -0800, Vipin Sharma wrote: > We should be fine without atomic64_t because we are using unsigned > long and not 64 bit explicitly. This will work on both 32 and 64 bit > machines. I see. > But I will add READ_ONCE and WRITE_ONCE because of potential chances of > load tearing and store tearing. > > Do you agree? Yes. > This was only here to avoid multiple reads of capacity and making sure > if condition and seq_print will see the same value. Aha. > Also, I was not aware of load and store tearing of properly aligned > and machine word size variables. I will add READ_ONCE and WRITE_ONCE > at other places. Yeah, although it's theoretical, I think it also serves well to annotate such unsychronized accesses. Thanks, Michal signature.asc Description: Digital signature
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On 3/12/21 2:51 PM, Sean Christopherson wrote:
On Fri, Mar 12, 2021, Vipin Sharma wrote:
On Thu, Mar 11, 2021 at 07:59:03PM +0100, Michal Koutný wrote:
+#ifndef CONFIG_KVM_AMD_SEV
+/*
+ * When this config is not defined, SEV feature is not supported and APIs in
+ * this file are not used but this file still gets compiled into the KVM AMD
+ * module.
+ *
+ * We will not have MISC_CG_RES_SEV and MISC_CG_RES_SEV_ES entries in the enum
+ * misc_res_type {} defined in linux/misc_cgroup.h.
BTW, was there any progress on conditioning sev.c build on
CONFIG_KVM_AMD_SEV? (So that the defines workaround isn't needeed.)
Tom, Brijesh,
Is this something you guys thought about or have some plans to do in the
future? Basically to not include sev.c in compilation if
CONFIG_KVM_AMD_SEV is disabled.
It's crossed my mind, but the number of stubs needed made me back off. I'm
certainly not opposed to the idea, it's just not a trivial change.
Right, I looked at it when I was doing the SEV-ES work and came to the
same conclusion.
Thanks,
Tom
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Fri, Mar 12, 2021, Vipin Sharma wrote:
> On Thu, Mar 11, 2021 at 07:59:03PM +0100, Michal Koutný wrote:
> > > +#ifndef CONFIG_KVM_AMD_SEV
> > > +/*
> > > + * When this config is not defined, SEV feature is not supported and
> > > APIs in
> > > + * this file are not used but this file still gets compiled into the KVM
> > > AMD
> > > + * module.
> > > + *
> > > + * We will not have MISC_CG_RES_SEV and MISC_CG_RES_SEV_ES entries in
> > > the enum
> > > + * misc_res_type {} defined in linux/misc_cgroup.h.
> > BTW, was there any progress on conditioning sev.c build on
> > CONFIG_KVM_AMD_SEV? (So that the defines workaround isn't needeed.)
>
> Tom, Brijesh,
> Is this something you guys thought about or have some plans to do in the
> future? Basically to not include sev.c in compilation if
> CONFIG_KVM_AMD_SEV is disabled.
It's crossed my mind, but the number of stubs needed made me back off. I'm
certainly not opposed to the idea, it's just not a trivial change.
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Thu, Mar 11, 2021 at 07:59:03PM +0100, Michal Koutný wrote:
> > +#ifndef CONFIG_KVM_AMD_SEV
> > +/*
> > + * When this config is not defined, SEV feature is not supported and APIs
> > in
> > + * this file are not used but this file still gets compiled into the KVM
> > AMD
> > + * module.
> > + *
> > + * We will not have MISC_CG_RES_SEV and MISC_CG_RES_SEV_ES entries in the
> > enum
> > + * misc_res_type {} defined in linux/misc_cgroup.h.
> BTW, was there any progress on conditioning sev.c build on
> CONFIG_KVM_AMD_SEV? (So that the defines workaround isn't needeed.)
Tom, Brijesh,
Is this something you guys thought about or have some plans to do in the
future? Basically to not include sev.c in compilation if
CONFIG_KVM_AMD_SEV is disabled.
Michal,
This should not be a blocker for misc controller. This thing can change
independent of misc cgroup.
Thanks
Vipin
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
On Thu, Mar 11, 2021 at 07:59:03PM +0100, Michal Koutný wrote:
> Given different two-fold nature (SEV caller vs misc controller) of some
> remarks below, I think it makes sense to split this into two patches:
> a) generic controller implementation,
> b) hooking the controller into SEV ASIDs management.
Sounds good. I will split it.
> > + if (misc_res_capacity[type])
> > + cg->res[type].max = max;
> In theory, parallel writers can clash here, so having the limit atomic
> type to prevent this would resolve it. See also commit a713af394cf3
> ("cgroup: pids: use atomic64_t for pids->limit").
We should be fine without atomic64_t because we are using unsigned
long and not 64 bit explicitly. This will work on both 32 and 64 bit
machines.
But I will add READ_ONCE and WRITE_ONCE because of potential chances of
load tearing and store tearing.
Do you agree?
> > +static int misc_cg_capacity_show(struct seq_file *sf, void *v)
> > +{
> > + int i;
> > + unsigned long cap;
> > +
> > + for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> > + cap = READ_ONCE(misc_res_capacity[i]);
> Why is READ_ONCE only here and not in other places that (actually) check
> against the set capacity value? Also, there should be a paired
> WRITE_ONCCE in misc_cg_set_capacity().
This was only here to avoid multiple reads of capacity and making sure
if condition and seq_print will see the same value. Also, I was not
aware of load and store tearing of properly aligned and machine word
size variables. I will add READ_ONCE and WRITE_ONCE at
other places.
Thanks
Vipin
Re: [Patch v3 1/2] cgroup: sev: Add misc cgroup controller
Hi Vipin.
On Thu, Mar 04, 2021 at 03:19:45PM -0800, Vipin Sharma
wrote:
> arch/x86/kvm/svm/sev.c| 65 +-
> arch/x86/kvm/svm/svm.h| 1 +
> include/linux/cgroup_subsys.h | 4 +
> include/linux/misc_cgroup.h | 130 +++
> init/Kconfig | 14 ++
> kernel/cgroup/Makefile| 1 +
> kernel/cgroup/misc.c | 402 ++
Given different two-fold nature (SEV caller vs misc controller) of some
remarks below, I think it makes sense to split this into two patches:
a) generic controller implementation,
b) hooking the controller into SEV ASIDs management.
> +#ifndef CONFIG_KVM_AMD_SEV
> +/*
> + * When this config is not defined, SEV feature is not supported and APIs in
> + * this file are not used but this file still gets compiled into the KVM AMD
> + * module.
> + *
> + * We will not have MISC_CG_RES_SEV and MISC_CG_RES_SEV_ES entries in the
> enum
> + * misc_res_type {} defined in linux/misc_cgroup.h.
BTW, was there any progress on conditioning sev.c build on
CONFIG_KVM_AMD_SEV? (So that the defines workaround isn't needeed.)
> static int sev_asid_new(struct kvm_sev_info *sev)
> {
> - int pos, min_asid, max_asid;
> + int pos, min_asid, max_asid, ret;
> bool retry = true;
> + enum misc_res_type type;
> +
> + type = sev->es_active ? MISC_CG_RES_SEV_ES : MISC_CG_RES_SEV;
> + sev->misc_cg = get_current_misc_cg();
> + ret = misc_cg_try_charge(type, sev->misc_cg, 1);
It may be safer to WARN_ON(sev->misc_cg) at this point (see below).
> [...]
> +e_uncharge:
> + misc_cg_uncharge(type, sev->misc_cg, 1);
> + put_misc_cg(sev->misc_cg);
> + return ret;
vvv
> @@ -140,6 +171,10 @@ static void sev_asid_free(int asid)
> }
>
> mutex_unlock(&sev_bitmap_lock);
> +
> + type = sev->es_active ? MISC_CG_RES_SEV_ES : MISC_CG_RES_SEV;
> + misc_cg_uncharge(type, sev->misc_cg, 1);
> + put_misc_cg(sev->misc_cg);
It may be safer to set sev->misc_cg to NULL here.
(IIUC, with current asid_{new,free} calls it shouldn't matter but why to
rely on it in the future.)
> +++ b/kernel/cgroup/misc.c
> [...]
> +static void misc_cg_reduce_charge(enum misc_res_type type, struct misc_cg
> *cg,
> + unsigned long amount)
misc_cg_cancel_charge seems to be a name more consistent with what we
already have in pids and memory controller.
> +static ssize_t misc_cg_max_write(struct kernfs_open_file *of, char *buf,
> + size_t nbytes, loff_t off)
> +{
> [...]
> +
> + if (!strcmp(MAX_STR, buf)) {
> + max = ULONG_MAX;
MAX_NUM for consistency with other places.
> + } else {
> + ret = kstrtoul(buf, 0, &max);
> + if (ret)
> + return ret;
> + }
> +
> + cg = css_misc(of_css(of));
> +
> + if (misc_res_capacity[type])
> + cg->res[type].max = max;
In theory, parallel writers can clash here, so having the limit atomic
type to prevent this would resolve it. See also commit a713af394cf3
("cgroup: pids: use atomic64_t for pids->limit").
> +static int misc_cg_current_show(struct seq_file *sf, void *v)
> +{
> + int i;
> + struct misc_cg *cg = css_misc(seq_css(sf));
> +
> + for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> + if (misc_res_capacity[i])
Since there can be some residual charges after removing capacity (before
draining), maybe the condition along the line
if (misc_res_capacity[i] || atomic_long_read(&cg->res[i].usage))
would be more informative for debugging.
> +static int misc_cg_capacity_show(struct seq_file *sf, void *v)
> +{
> + int i;
> + unsigned long cap;
> +
> + for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> + cap = READ_ONCE(misc_res_capacity[i]);
Why is READ_ONCE only here and not in other places that (actually) check
against the set capacity value? Also, there should be a paired
WRITE_ONCCE in misc_cg_set_capacity().
Thanks,
Michal
signature.asc
Description: Digital signature
