Re: [PATCH 00/11] scsi: libsas: Remove in_interrupt() check

[John Garry]

On 18/12/2020 20:43, Ahmed S. Darwish wrote:
> Folks,
>
> In the discussion about preempt count consistency across kernel
> configurations:
>
>    https://lkml.kernel.org/r/20200914204209.256266093@xxxxxxxxxxxxx
>
> it was concluded that the usage of in_interrupt() and related context
> checks should be removed from non-core code.
>
> This includes memory allocation mode decisions (GFP_*). In the long run,
> usage of in_interrupt() and its siblings should be banned from driver
> code completely.
>
> This series addresses SCSI libsas. Basically, the function:
>
>    => drivers/scsi/libsas/sas_init.c:
>    struct asd_sas_event *sas_alloc_event(struct asd_sas_phy *phy)
>    {
>          ...
>          gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
>          event = kmem_cache_zalloc(sas_event_cache, flags);

Hi Ahmed,

Firstly I would say that it would be nice to just remove all the atomic 
context calls. But that may require significant LLDD rework and 
participation from driver stakeholders.

However, considering function sas_alloc_event() again:

	gfp_t flags = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;

	...

	event = kmem_cache_zalloc(sas_event_cache, flags);
	if (!event)
		return NULL;

	atomic_inc(&phy->event_nr);

	if (atomic_read(&phy->event_nr) > phy->ha->event_thres) {
		/* Code to shutdown the phy */
	}

	return event;


So default for phy->ha->event_thres is 32, and I can't imagine that 
anyone has ever reconfigured this via sysfs or even required a value 
that large. Maybe Jason (cc'ed) knows better. It's an arbitrary value to 
say that the PHY is malfunctioning. I do note that there is the circular 
path sas_alloc_event() -> sas_notify_phy_event() -> sas_alloc_event() 
there also.

Anyway, if the 32x event memories were per-allocated, maybe there is a 
clean method to manage this memory, which even works in atomic context, 
so we could avoid this rework (ignoring the context bugs you reported 
for a moment). I do also note that the sas_event_cache size is not huge.

Anyway, I'll look at the rest of the series.

Thanks,
John

>          ...
>    }
>
> is transformed so that callers explicitly pass the gfp_t memory
> allocation flags. Affected libsas clients are modified accordingly.
>
> The first six patches have "Fixes: " tags and address bugs the were
> noticed during the context analysis.
>
> Thanks!
>
> 8<--------------
>
> Ahmed S. Darwish (11):
>    Documentation: scsi: libsas: Remove notify_ha_event()
>    scsi: libsas: Introduce a _gfp() variant of event notifiers
>    scsi: mvsas: Pass gfp_t flags to libsas event notifiers
>    scsi: isci: port: link down: Pass gfp_t flags
>    scsi: isci: port: link up: Pass gfp_t flags
>    scsi: isci: port: broadcast change: Pass gfp_t flags
>    scsi: libsas: Pass gfp_t flags to event notifiers
>    scsi: pm80xx: Pass gfp_t flags to libsas event notifiers
>    scsi: aic94xx: Pass gfp_t flags to libsas event notifiers
>    scsi: hisi_sas: Pass gfp_t flags to libsas event notifiers
>    scsi: libsas: event notifiers: Remove non _gfp() variants
>
>   Documentation/scsi/libsas.rst          |  5 ++--
>   drivers/scsi/aic94xx/aic94xx_scb.c     | 18 ++++++------
>   drivers/scsi/hisi_sas/hisi_sas.h       |  3 +-
>   drivers/scsi/hisi_sas/hisi_sas_main.c  | 26 ++++++++++--------
>   drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  5 ++--
>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  5 ++--
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  5 ++--
>   drivers/scsi/isci/port.c               | 14 ++++++----
>   drivers/scsi/libsas/sas_event.c        | 21 ++++++++------
>   drivers/scsi/libsas/sas_init.c         | 11 ++++----
>   drivers/scsi/libsas/sas_internal.h     |  4 +--
>   drivers/scsi/mvsas/mv_sas.c            | 22 +++++++--------
>   drivers/scsi/pm8001/pm8001_hwi.c       | 38 +++++++++++++-------------
>   drivers/scsi/pm8001/pm8001_sas.c       |  8 +++---
>   drivers/scsi/pm8001/pm80xx_hwi.c       | 30 ++++++++++----------
>   include/scsi/libsas.h                  |  4 +--
>   16 files changed, 116 insertions(+), 103 deletions(-)
>
> base-commit: 2c85ebc57b3e1817b6ce1a6b703928e113a90442
> --
> 2.29.2
> .