linux-kernel - Re: [PATCH V3 5/7] genirq/affinity: move group_cpus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <74bcc75e-0b68-1d6b-b7f6-4681ec754257@huawei.com>
Date:   Wed, 29 Sep 2021 15:40:44 +0100
From:   John Garry <john.garry@...wei.com>
To:     Ming Lei <ming.lei@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Jens Axboe" <axboe@...nel.dk>
CC:     <linux-kernel@...r.kernel.org>, <linux-block@...r.kernel.org>,
        "Christoph Hellwig" <hch@....de>
Subject: Re: [PATCH V3 5/7] genirq/affinity: move group_cpus_evenly() into
 lib/


> +/**
> + * group_cpus_evenly - Group all CPUs evenly per NUMA/CPU locality
> + * @numgrps: number of groups
> + *
> + * Return: cpumask array if successful, NULL otherwise. And each element
> + * includes CPUs assigned to this group
> + *
> + * Try to put close CPUs from viewpoint of CPU and NUMA locality into
> + * same group, and run two-stage grouping:
> + *	1) allocate present CPUs on these groups evenly first
> + *	2) allocate other possible CPUs on these groups evenly
> + *
> + * We guarantee in the resulted grouping that all CPUs are covered, and
> + * no same CPU is assigned to different groups

nit: I'd have "no same CPU is assigned to multiple groups"

> + */
> +struct cpumask *group_cpus_evenly(unsigned int numgrps)

nit: The name group_cpus_evenly() would imply an action on some cpus, 
when it's just calculating some masks - I think "masks" should be at 
least included in the name

> +{
> +	unsigned int curgrp = 0, nr_present = 0, nr_others = 0;
> +	cpumask_var_t *node_to_cpumask;
> +	cpumask_var_t nmsk, npresmsk;
> +	int ret = -ENOMEM;
> +	struct cpumask *masks = NULL;
> +
> +	if (!zalloc_cpumask_var(&nmsk, GFP_KERNEL))
> +		return NULL;
> +
> +	if (!zalloc_cpumask_var(&npresmsk, GFP_KERNEL))
> +		goto fail_nmsk;
> +
> +	node_to_cpumask = alloc_node_to_cpumask();
> +	if (!node_to_cpumask)
> +		goto fail_npresmsk;
> +
> +	masks = kcalloc(numgrps, sizeof(*masks), GFP_KERNEL);
> +	if (!masks)
> +		goto fail_node_to_cpumask;
> +
> +	/* Stabilize the cpumasks */
> +	cpus_read_lock();
> +	build_node_to_cpumask(node_to_cpumask);
> +
> +	/* grouping present CPUs first */
> +	ret = __group_cpus_evenly(curgrp, numgrps, node_to_cpumask,
> +				  cpu_present_mask, nmsk, masks);
> +	if (ret < 0)
> +		goto fail_build_affinity;
> +	nr_present = ret;
> +
> +	/*
> +	 * Allocate non present CPUs starting from the next group to be
> +	 * handled. If the grouping of present CPUs already exhausted the
> +	 * group space, assign the non present CPUs to the already
> +	 * allocated out groups.
> +	 */
> +	if (nr_present >= numgrps)
> +		curgrp = 0;
> +	else
> +		curgrp = nr_present;
> +	cpumask_andnot(npresmsk, cpu_possible_mask, cpu_present_mask);
> +	ret = __group_cpus_evenly(curgrp, numgrps, node_to_cpumask,
> +				  npresmsk, nmsk, masks);
> +	if (ret >= 0)
> +		nr_others = ret;
> +
> + fail_build_affinity:

nit: Strange that success path goes through "fail" labels. Current code 
is this way, so feel free to ignore.

> +	cpus_read_unlock();
> +
> +	if (ret >= 0)
> +		WARN_ON(nr_present + nr_others < numgrps);
> +
> + fail_node_to_cpumask:
> +	free_node_to_cpumask(node_to_cpumask);
> +
> + fail_npresmsk:
> +	free_cpumask_var(npresmsk);
> +
> + fail_nmsk:
> +	free_cpumask_var(nmsk);
> +	if (ret < 0) {
> +		kfree(masks);
> +		return NULL;
> +	}
> +	return masks;
> +}
> +EXPORT_SYMBOL_GPL(group_cpus_evenly);

Are there any users which are available as modules? As I see, the only 
users are blk-mq-cpumap.c and irq/affinity.c, which I guess aren't 
available as modules.

> 

Thanks,
John