On 21.02.2012 15:18, James Bottomley wrote:
> On Tue, 2012-02-21 at 10:01 +0100, Jan Schmidt wrote:
>> Hi Eric,
>>
>> On 21.02.2012 00:06, Moore, Eric wrote:
>>> Jan - the smp_processor_id() is part of NUMA support which I added over a year ago. I don't see any problem with the driver running on sles11sp2 which has btrfs support ( I'm not sure if CONFIG_DEBUG_PREEMPT is turned on as I have not checked). Any theory why its oopsing in smp_processor_id() ?
>>
>> -- excerpt from include/linux/smp.h --
>> /*
>> * smp_processor_id(): get the current CPU ID.
>> *
>> * if DEBUG_PREEMPT is enabled then we check whether it is
>> * used in a preemption-safe way. (smp_processor_id() is safe
>> * if it's used in a preemption-off critical section, or in
>> * a thread that is bound to the current CPU.)
>> [...]
>> */
>> #ifdef CONFIG_DEBUG_PREEMPT
>> extern unsigned int debug_smp_processor_id(void);
>> # define smp_processor_id() debug_smp_processor_id()
>> #else
>> # define smp_processor_id() raw_smp_processor_id()
>> #endif
>>
>> #define get_cpu() ({ preempt_disable(); smp_processor_id(); })
>> #define put_cpu() preempt_enable()
>> -- end --
>>
>> ... and ...
>>
>> -- excerpt from drivers/scsi/mpt2sas/mpt2sas_base.c --
>> static inline u8
>> _base_get_msix_index(struct MPT2SAS_ADAPTER *ioc)
>> {
>> return ioc->cpu_msix_table[smp_processor_id()];
>> }
>> -- end --
>>
>> Suggestion: Use get_cpu() and put_cpu().
>
> you mean
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c
> index 0b2c955..5235068 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
> @@ -1785,7 +1785,9 @@ static inline void _base_writeq(__u64 b, volatile void __iomem *addr,
> static inline u8
> _base_get_msix_index(struct MPT2SAS_ADAPTER *ioc)
> {
> - return ioc->cpu_msix_table[smp_processor_id()];
> + int cpu = get_cpu();
> + put_cpu();
> + return ioc->cpu_msix_table[cpu];
> }
>
> /**
>
>
> This is the I/O hot path we're talking about. The point about this use
> of smp_processor_id() is that if the block layer doesn't specify a CPU
> affinity for the request and we're using MSI interrupt steering, we have
> to (because the MSIs steer to different CPUs), so we just fill in the
> approximation of the current CPU ... we don't care that it be exactly
> accurate at the time. We need the lightest weight way of getting
> current CPU (and we don't care if it changes because of pre-empt at the
> next instruction).
Okay, maybe I [...]-ed out the most important part of the comment from
include/linux/smp.h then. It says:
[...]
* NOTE: raw_smp_processor_id() is for internal use only
* (smp_processor_id() is the preferred variant), but in rare
* instances it might also be used to turn off false positives
* (i.e. smp_processor_id() use that the debugging code reports but
* which use for some reason is legal). Don't use this to hack around
* the warning message, as your code might not work under PREEMPT.
*/
For this case, raw_smp_processor_id() seems to be the best choice.
Thanks!
-Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
