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

Ben Hutchings
2008-12-20T23:52:21Z

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.