netdev - Re: [PATCH net-next V4 1/2] net: Utility function to get affinity

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: <1394399438.15968.45.camel@deadeye.wl.decadent.org.uk>
Date:	Sun, 09 Mar 2014 21:10:38 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Amir Vadai <amirv@...lanox.com>
Cc:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Govindarajulu Varadarajan <gvaradar@...co.com>
Subject: Re: [PATCH net-next V4 1/2] net: Utility function to get
 affinity_hint by policy

On Sun, 2014-03-09 at 09:39 +0200, Amir Vadai wrote:
> This function sets the affinity_mask according to a numa aware policy.
> affinity_mask could be used as an affinity hint for the IRQ related to
> this rx queue.
> Current policy is to spread rx queues accross cores - local cores first.
> It could be extended in the future.
[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2117,6 +2117,63 @@ EXPORT_SYMBOL(netif_set_real_num_rx_queues);
>  #endif
>  
>  /**
> + * netif_set_rx_queue_affinity_hint - set affinity hint of rx queue
> + * @rxq: index of rx queue

Well, there is nothing net-specific here, so it could be a queue index
within any multiqueue device, right?

If this doesn't specifically relate to net devices, it probably belongs
in genirq (kernel/irq/*) and you should send this to Thomas Gleixner and
LKML too.

> + * @numa_node: prefered numa_node
> + * @affinity_mask: the relevant cpu bit is set according to the policy
> +
> + * This function sets the affinity_mask according to a numa aware policy.
> + * affinity_mask coulbe used as an affinity hint for the IRQ related to this

Typo: coulbe => could be

> + * rx queue.
> + * The policy is to spread rx queues accross cores - local cores first.

Typo: accross => across

> + *
> + * Returns 0 on success, or a negative error code.
> + */
> +int netif_set_rx_queue_affinity_hint(int rxq, int numa_node,
> +				     cpumask_var_t affinity_mask)

cpumask_var_t is for local variable definitions; you should use
cpumask_t * here.

> +{
> +	const struct cpumask *p_numa_cores_mask;
> +	cpumask_var_t non_numa_cores_mask = NULL;

This may be a syntax error - cpumask_var_t can be either a pointer or an
array, depending on the value of NR_CPUS.

I think you should either add a flag to indicate whether this has been
initialised, or restructure the code so that that's implicit.

> +	int affinity_cpu;
> +	int ret = 0;
> +
> +	rxq %= num_online_cpus();
> +
> +	p_numa_cores_mask = cpumask_of_node(numa_node);
> +	if (!p_numa_cores_mask)
> +		p_numa_cores_mask = cpu_online_mask;
> +
> +	for_each_cpu(affinity_cpu, p_numa_cores_mask) {
> +		if (--rxq < 0)
> +			goto out;
> +	}
[...]

What if some CPUs on the device's node are offline?

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)