Re: [PATCH 1/4] qla2xxx: Use raw_smp_processor_id() where appropriate

[Daniel Wagner <dwagner@suse.de>]

Hi Bart,

On Sun, Mar 01, 2020 at 07:30:20PM -0800, Bart Van Assche wrote:
> This patch fixes e.g. the following kernel complaint:

[...]

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 5b2deaa730bf..582fc5dcc98c 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -8992,7 +8992,7 @@ struct qla_qpair *qla2xxx_create_qpair(struct scsi_qla_host *vha, int qos,
>  		qpair->rsp->req = qpair->req;
>  		qpair->rsp->qpair = qpair;
>  		/* init qpair to this cpu. Will adjust at run time. */
> -		qla_cpu_update(qpair, smp_processor_id());
> +		qla_cpu_update(qpair, raw_smp_processor_id());

When this is updated later anway, why not inializing with a invalid id
from beginning? Because using raw_smp_processor_id() is just silencing
the report without addressing the problem.

>  		if (IS_T10_PI_CAPABLE(ha) && ql2xenabledif) {
>  			if (ha->fw_attributes & BIT_4)
> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 8d7a905f6247..a5aae276fbb2 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -3217,8 +3217,8 @@ void qla24xx_process_response_queue(struct scsi_qla_host *vha,
>  	if (!ha->flags.fw_started)
>  		return;
>  
> -	if (rsp->qpair->cpuid != smp_processor_id())
> -		qla_cpu_update(rsp->qpair, smp_processor_id());
> +	if (rsp->qpair->cpuid != raw_smp_processor_id())
> +		qla_cpu_update(rsp->qpair, raw_smp_processor_id());

If I read it correctly, qla24xx_processor_response_queue() can be
called from either IRQ context or from a threading context. So that
means complaint that smp_processor_id() is used in preemptable context
is correct. So again this is just silencing it.

>  
>  	while (rsp->ring_ptr->signature != RESPONSE_PROCESSED) {
>  		pkt = (struct sts_entry_24xx *)rsp->ring_ptr;
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 622e7337affc..c4c6a8e1b46d 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -4368,7 +4368,7 @@ static int qlt_handle_cmd_for_atio(struct scsi_qla_host *vha,
>  		queue_work_on(cmd->se_cmd.cpuid, qla_tgt_wq, &cmd->work);
>  	} else if (ha->msix_count) {
>  		if (cmd->atio.u.isp24.fcp_cmnd.rddata)
> -			queue_work_on(smp_processor_id(), qla_tgt_wq,
> +			queue_work_on(raw_smp_processor_id(), qla_tgt_wq,

As I understand the code this is the main reason why the code tries to
figure out on which CPU it is running. The first part of the
processing happens either in IRQ context or in the threading context
in which qla24xx_process_response_queue() runs and now the second part
of the processing happens in the work queue. And this code tries to
stick the processing context on the same CPU to avoid cash trashing.

At creation time of the qpair queues, the CPU on which they should be
processed should be noted. And in this code path we just would look up
on which queue we are and which CPU should be used.

This would also avoid the qla_cpu_update() call in
qla24xx_process_response_queue().

So far the theory, I tried to figure out how to implement this scheme
but I'm still strungling with the code base.

Thanks,
Daniel