Re: [RFC 1/2] cgroup: sev: Add misc cgroup controller
From: Vipin Sharma
Date: Wed Feb 24 2021 - 23:58:47 EST
On Tue, Feb 23, 2021 at 07:24:55PM +0100, Michal Koutný wrote:
>
On Thu, Feb 18, 2021 at 11:55:48AM -0800, Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
>
> --- a/arch/x86/kvm/svm/sev.c
>
> +++ b/arch/x86/kvm/svm/sev.c
>
> [...]
>
> +#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.
>
I'm not familiar with the layout of KVM/SEV compile targets but wouldn't
>
it be simpler to exclude whole svm/sev.c when !CONFIG_KVM_AMD_SEV?
>
Tom,
Is there any plan to exclude sev.c compilation if CONFIG_KVM_AMD_SEV is
not set?
>
> +++ b/kernel/cgroup/misc.c
>
> [...]
>
> +/**
>
> + * misc_cg_set_capacity() - Set the capacity of the misc cgroup res.
>
> + * @type: Type of the misc res.
>
> + * @capacity: Supported capacity of the misc res on the host.
>
> + *
>
> + * If capacity is 0 then the charging a misc cgroup fails for that type.
>
> + *
>
> + * The caller must serialize invocations on the same resource.
>
> + *
>
> + * Context: Process context.
>
> + * Return:
>
> + * * %0 - Successfully registered the capacity.
>
> + * * %-EINVAL - If @type is invalid.
>
> + * * %-EBUSY - If current usage is more than the capacity.
>
When is this function supposed to be called? At boot only or is this
>
meant for some kind of hot unplug functionality too?
>
This function is meant for hot unplug functionality too.
>
> +int misc_cg_try_charge(enum misc_res_type type, struct misc_cg **cg,
>
> + unsigned int amount)
>
> [...]
>
> + new_usage = atomic_add_return(amount, &res->usage);
>
> + if (new_usage > res->max ||
>
> + new_usage > misc_res_capacity[type]) {
>
> + ret = -EBUSY;
>
I'm not sure the user of this resource accounting will always be able to
>
interpret EBUSY returned from depths of the subsystem.
>
See what's done in pids controller in order to give some useful
>
information about why operation failed.
Just to be on the same page are you talking about adding an events file
like in pids?
>
>
> + goto err_charge;
>
> + }
>
> +
>
> + // First one to charge gets a reference.
>
> + if (new_usage == amount)
>
> + css_get(&i->css);
>
1) Use the /* comment */ style.
>
2) You pin the whole path from task_cg up to root (on the first charge).
>
That's unnecessary since children reference their parents.
>
Also why do you get the reference only for the first charger? While it
>
may work, it seems too convoluted to me.
>
It'd be worth documenting what the caller can expect wrt to ref count of
>
the returned misc_cg.
Suppose a user charges 5 resources in a single charge call but uncharges
them in 5 separate calls one by one. I cannot take reference on every
charge and put the reference for every uncharge as it is not guaranteed
to have equal number of charge-uncharge pairs and we will end up with
the wrong ref count.
However, if I take reference at the first charge and remove reference at
last uncharge then I can keep the ref count in correct sync.
I can rewrite if condition to (new_usage == amount && task_cg == i)
this will avoid pinning whole path up to the root. I was thinking that
original code was simpler, clearly I was wrong.
Thanks
Vipin