Re: [PATCH 07/10] scsi/esas2r: Replace tasklet with workqueue

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2022-05-30 16:15:09 [-0700], Davidlohr Bueso wrote:
> diff --git a/drivers/scsi/esas2r/esas2r_int.c b/drivers/scsi/esas2r/esas2r_int.c
> index 5281d9356327..1b1b8b65539d 100644
> --- a/drivers/scsi/esas2r/esas2r_int.c
> +++ b/drivers/scsi/esas2r/esas2r_int.c
> @@ -86,7 +86,7 @@ void esas2r_polled_interrupt(struct esas2r_adapter *a)
>  
>  /*
>   * Legacy and MSI interrupt handlers.  Note that the legacy interrupt handler
> - * schedules a TASKLET to process events, whereas the MSI handler just
> + * schedules work to process events, whereas the MSI handler just
>   * processes interrupt events directly.

Yeah, and why? What is special about the legacy vs MSI that different
behaviour is needed. Why not do the tasklet/workqueue thingy for MSI,
too?

>   */
>  irqreturn_t esas2r_interrupt(int irq, void *dev_id)
> diff --git a/drivers/scsi/esas2r/esas2r_main.c b/drivers/scsi/esas2r/esas2r_main.c
> index 7a4eadad23d7..abe45a934cce 100644
> --- a/drivers/scsi/esas2r/esas2r_main.c
> +++ b/drivers/scsi/esas2r/esas2r_main.c
> @@ -1540,10 +1550,10 @@ void esas2r_complete_request_cb(struct esas2r_adapter *a,
>  	esas2r_free_request(a, rq);
>  }
>  
> -/* Run tasklet to handle stuff outside of interrupt context. */
> -void esas2r_adapter_tasklet(unsigned long context)
> +/* Handle stuff outside of interrupt context. */
> +void esas2r_adapter_work(struct work_struct *work)
>  {
> -	struct esas2r_adapter *a = (struct esas2r_adapter *)context;
> +	struct esas2r_adapter *a = (struct esas2r_adapter *)work;

container_of()

>  
>  	if (unlikely(test_bit(AF2_TIMER_TICK, &a->flags2))) {
>  		clear_bit(AF2_TIMER_TICK, &a->flags2);
> @@ -1555,16 +1565,16 @@ void esas2r_adapter_tasklet(unsigned long context)
>  		esas2r_adapter_interrupt(a);
>  	}
>  
> -	if (esas2r_is_tasklet_pending(a))
> -		esas2r_do_tasklet_tasks(a);
> +	if (esas2r_is_work_pending(a))
> +		esas2r_do_work_tasks(a);
>  
> -	if (esas2r_is_tasklet_pending(a)
> +	if (esas2r_is_work_pending(a)
>  	    || (test_bit(AF2_INT_PENDING, &a->flags2))
>  	    || (test_bit(AF2_TIMER_TICK, &a->flags2))) {
> -		clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
> -		esas2r_schedule_tasklet(a);
> +		clear_bit(AF_WORK_SCHEDULED, &a->flags);
> +		esas2r_schedule_work(a);

This AF_TASKLET_SCHEDULED bit is odd and shouldn't be needed. What you
usually do is set_bit() + schedule_tasklet() and clear_bit() within the
tasklet. If the tasklet is already running then it will be scheduled
again which is intended. If it is not yet running then it will run once.

>  	} else {
> -		clear_bit(AF_TASKLET_SCHEDULED, &a->flags);
> +		clear_bit(AF_WORK_SCHEDULED, &a->flags);
>  	}
>  }
>  
> @@ -1586,7 +1596,7 @@ static void esas2r_timer_callback(struct timer_list *t)
>  
>  	set_bit(AF2_TIMER_TICK, &a->flags2);
>  
> -	esas2r_schedule_tasklet(a);
> +	esas2r_schedule_work(a);
>  
>  	esas2r_kickoff_timer(a);

It appears that the timer is always scheduled (except in degraded
mode) and the timer re-arms itself and schedules the tasklet. The timer
and the tasklet are synchronized and the tasklet will always after the
timer and one can not interrupt the other. But with the workqueue you
can get the following depending on how much the workqueue was delayed:

       worker                                                timer
   esas2r_adapter_work();
    -> test_bit(AF2_TIMER_TICK, &a->flags2) -> false
                                                            esas2r_timer_callback
                                                            set_bit(AF2_TIMER_TICK, &a->flags2);
                                                            esas2r_schedule_tasklet()
                                                            -> if (!test_and_set_bit(AF_TASKLET_SCHEDULED, &a->flags)) (bit set)
                                                               no schedule since bit was set
                                                            esas2r_kickoff_timer();
       clear_bit(AF_TASKLET_SCHEDULED, &a->flags);

No more tasklets. So another reason to get rid of this
AF_TASKLET_SCHEDULED bit ;)

>  }

Sebastian