linux-kernel - Re: [PATCH 7/8] cpumask: convert misc driver functions

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: <1229815359.2723.19.camel@hashbaz.i.decadent.org.uk>
Date:	Sat, 20 Dec 2008 23:22:39 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Mike Travis <travis@....com>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	linux-kernel@...r.kernel.org, Dean Nelson <dcn@....com>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	Robert Richter <robert.richter@....com>
Subject: Re: [PATCH 7/8] cpumask: convert misc driver functions

On Fri, 2008-12-19 at 08:01 -0800, Mike Travis wrote:
[...]
> --- linux-2.6-for-ingo.orig/drivers/net/sfc/efx.c
> +++ linux-2.6-for-ingo/drivers/net/sfc/efx.c
> @@ -809,19 +809,18 @@ static void efx_fini_io(struct efx_nic *
>  /* Get number of RX queues wanted.  Return number of online CPU
>   * packages in the expectation that an IRQ balancer will spread
>   * interrupts across them. */
> -static int efx_wanted_rx_queues(void)
> +static int efx_wanted_rx_queues(struct cpumask *scratch)
>  {
> -	cpumask_t core_mask;
>  	int count;
>  	int cpu;
>  
> -	cpus_clear(core_mask);
> +	cpumask_clear(scratch);
>  	count = 0;
>  	for_each_online_cpu(cpu) {
> -		if (!cpu_isset(cpu, core_mask)) {
> +		if (!cpumask_test_cpu(cpu, scratch)) {
>  			++count;
> -			cpus_or(core_mask, core_mask,
> -				topology_core_siblings(cpu));
> +			cpumask_or(scratch, scratch,
> +				   topology_core_cpumask(cpu));
>  		}
>  	}
>  
> @@ -831,7 +830,7 @@ static int efx_wanted_rx_queues(void)
>  /* Probe the number and type of interrupts we are able to obtain, and
>   * the resulting numbers of channels and RX queues.
>   */
> -static void efx_probe_interrupts(struct efx_nic *efx)
> +static void efx_probe_interrupts(struct efx_nic *efx, struct cpumask *scratch)
>  {
>  	int max_channels =
>  		min_t(int, efx->type->phys_addr_channels, EFX_MAX_CHANNELS);
> @@ -845,7 +844,8 @@ static void efx_probe_interrupts(struct 
>  		 * (or as specified by the rss_cpus module parameter).
>  		 * We will need one channel per interrupt.
>  		 */
> -		wanted_ints = rss_cpus ? rss_cpus : efx_wanted_rx_queues();
> +		wanted_ints = rss_cpus ? rss_cpus :
> +					 efx_wanted_rx_queues(scratch);
>  		efx->n_rx_queues = min(wanted_ints, max_channels);
>  
>  		for (i = 0; i < efx->n_rx_queues; i++)
> @@ -923,24 +923,29 @@ static void efx_set_channels(struct efx_
>  static int efx_probe_nic(struct efx_nic *efx)
>  {
>  	int rc;
> +	cpumask_var_t scratch;
>  
>  	EFX_LOG(efx, "creating NIC\n");
>  
> +	if (!alloc_cpumask_var(&scratch, GFP_KERNEL))
> +		return -ENOMEM;
> +
>  	/* Carry out hardware-type specific initialisation */
>  	rc = falcon_probe_nic(efx);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	/* Determine the number of channels and RX queues by trying to hook
>  	 * in MSI-X interrupts. */
> -	efx_probe_interrupts(efx);
> +	efx_probe_interrupts(efx, scratch);
>  
>  	efx_set_channels(efx);
>  
>  	/* Initialise the interrupt moderation settings */
>  	efx_init_irq_moderation(efx, tx_irq_mod_usec, rx_irq_mod_usec);
> -
> -	return 0;
> +out:
> +	free_cpumask_var(scratch);
> +	return rc;
>  }
>  
>  static void efx_remove_nic(struct efx_nic *efx)
[...]

I assume you're moving the allocation up to efx_probe_nic() because
efx_wanted_rx_queues() and efx_probe_interrupts() cannot return failure.
It really isn't worth exposing that detail up the call chain though.  I
think it's acceptable for efx_wanted_rx_queues() to log an error message
and return 1 in the exceedingly unlikely case that the allocation fails.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/