Re: [PATCH 1/7] aoe: support more AoE addresses with dynamic blockdevice minor numbers

[Ed Cashin]

On Oct 2, 2012, at 5:12 PM, Andrew Morton wrote:
...
>> +static int
>> +minor_get(ulong *minor)
>> {
>> -	struct aoedev *d;
>> 	ulong flags;
>> +	ulong n;
>> +	int error = 0;
>> +
>> +	spin_lock_irqsave(&used_minors_lock, flags);
>> +	n = find_first_zero_bit(used_minors, N_DEVS);
>> +	if (n < N_DEVS)
>> +		set_bit(n, used_minors);
>> +	else
>> +		error = -1;
>> +	spin_unlock_irqrestore(&used_minors_lock, flags);
>> +
>> +	*minor = n * AOE_PARTITIONS;
>> +	return error;
>> +}
> 
> - can use the more efficient __set_bit() inside that spinlock.

Thanks for that observation.  Because this operation occurs on target discovery, which is expected to be relatively infrequent, my inclination is to leave it in its atomic form, though, and leave the __set_bit() for another time when optimization is needed.  Like you said, this is a minor point.  I wouldn't mind changing it, though, if you think it's worth me resubmitting the patch.  Just let me know.

> - could avoid setting *minor if we're returning an error.

Yes.  The only caller of aoedev.c:minor_get() handles that correctly.  Again, just let me know if you think this is worth a resubmission of the patch.  Otherwise I'll just make a note to myself to try to avoid setting output parameters on error in the future.

-- 
  Ed Cashin
  ecashin@xxxxxxxxxx


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