Re: [PATCH 10/12] xen/hypfs: add cpupool directories

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.10.2020 10:13, Juergen Gross wrote:
> @@ -992,6 +994,78 @@ static struct notifier_block cpu_nfb = {
>      .notifier_call = cpu_callback
>  };
>  
> +#ifdef CONFIG_HYPFS
> +static HYPFS_DIR_INIT(cpupool_pooldir, "id");

This "id" string won't appear anywhere, will it? I would have
expected this to act as the format string used when generating
names of the dynamic entries. This would e.g. allow CPU pools
to have decimal numbered names, but other entries also hex
ones, and then if so desired also e.g. with leading zeros.

> +static int cpupool_dir_read(const struct hypfs_entry *entry,
> +                            XEN_GUEST_HANDLE_PARAM(void) uaddr)
> +{
> +    int ret = 0;
> +    struct cpupool **q;

I was going to ask for const here, but the way for_each_cpupool()
works looks to prohibit this. Nevertheless I wonder whether the
extra level of indirection there wouldn't better be dropped. Of
the users, only cpupool_destroy() looks to need it, so open-
coding the loop there (or introducing an auxiliary variable)
would allow improvements here and elsewhere. (Actually I notice
there's also a similar use in cpupool_create(), but the general
consideration remains.)

> +    spin_lock(&cpupool_lock);
> +
> +    for_each_cpupool(q)
> +    {
> +        ret = hypfs_read_dyndir_id_entry(&cpupool_pooldir, (*q)->cpupool_id,
> +                                         !(*q)->next, &uaddr);
> +        if ( ret )
> +            break;
> +    }
> +
> +    spin_unlock(&cpupool_lock);
> +
> +    return ret;
> +}
> +
> +static unsigned int cpupool_dir_getsize(const struct hypfs_entry *entry)
> +{
> +    struct cpupool **q;
> +    unsigned int size = 0;
> +
> +    spin_lock(&cpupool_lock);
> +
> +    for_each_cpupool(q)
> +        size += HYPFS_DIRENTRY_SIZE(snprintf(NULL, 0, "%d", 
> (*q)->cpupool_id));

Beyond the remark above I consider this problematic: If the pool
ID was negative, the use of %d here would get things out of sync
with the %u uses in hypfs.c. I guess exposing
HYPFS_DIRENTRY_SIZE() isn't the right approach, and you instead
need another hypfs library function.

> +static struct hypfs_entry *cpupool_dir_findentry(struct hypfs_entry_dir *dir,
> +                                                 const char *name,
> +                                                 unsigned int name_len)
> +{
> +    unsigned long id;
> +    const char *end;
> +    struct cpupool *cpupool;
> +
> +    id = simple_strtoul(name, &end, 10);
> +    if ( id > INT_MAX || end != name + name_len )

What does this INT_MAX match up with? Afaics
XEN_SYSCTL_CPUPOOL_OP_CREATE is fine to have an effectively
negative pool ID passed in (the public interface struct uses
uint32_t, but this gets converted to plain int first thing in
the sysctl handler).

> +        return ERR_PTR(-ENOENT);
> +
> +    spin_lock(&cpupool_lock);
> +
> +    cpupool = __cpupool_find_by_id(id, true);
> +
> +    spin_unlock(&cpupool_lock);
> +
> +    if ( !cpupool )
> +        return ERR_PTR(-ENOENT);
> +
> +    return hypfs_gen_dyndir_entry_id(&cpupool_pooldir, id);

The latest this one makes clear that cpupool_lock nests inside
the hypfs one. I think this wants spelling out next to the
definition of the former, as it implies that there are
restrictions on what can be done from inside cpupool-locked
regions. hypfs_read_dyndir_id_entry(), for example, has to
remain lock free for this reason.

Jan