LKML Archive on lore.kernel.org
 help / color / Atom feed
From: John Garry <john.garry@huawei.com>
To: "Ahmed S. Darwish" <a.darwish@linutronix.de>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Daniel Wagner <dwagner@suse.de>, Jason Yan <yanaijie@huawei.com>,
	"Artur Paszkiewicz" <artur.paszkiewicz@intel.com>,
	Jack Wang <jinpu.wang@cloud.ionos.com>
Cc: <linux-scsi@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Sebastian A. Siewior" <bigeasy@linutronix.de>,
	Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH 00/11] scsi: libsas: Remove in_interrupt() check
Date: Mon, 21 Dec 2020 10:13:42 +0000
Message-ID: <9674052e-3deb-2a45-6082-4a40a472a219@huawei.com> (raw)
In-Reply-To: <20201218204354.586951-1-a.darwish@linutronix.de>

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@linutronix.de
> 
> 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
> .
> 


  parent reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-18 20:43 Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 01/11] Documentation: scsi: libsas: Remove notify_ha_event() Ahmed S. Darwish
2020-12-21  9:10   ` John Garry
2020-12-18 20:43 ` [PATCH 02/11] scsi: libsas: Introduce a _gfp() variant of event notifiers Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 03/11] scsi: mvsas: Pass gfp_t flags to libsas " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 04/11] scsi: isci: port: link down: Pass gfp_t flags Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 05/11] scsi: isci: port: link up: " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 06/11] scsi: isci: port: broadcast change: " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 07/11] scsi: libsas: Pass gfp_t flags to event notifiers Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 08/11] scsi: pm80xx: Pass gfp_t flags to libsas " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 09/11] scsi: aic94xx: " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 10/11] scsi: hisi_sas: " Ahmed S. Darwish
2020-12-18 20:43 ` [PATCH 11/11] scsi: libsas: event notifiers: Remove non _gfp() variants Ahmed S. Darwish
2020-12-21 17:17   ` John Garry
2020-12-21 17:38     ` Ahmed S. Darwish
2020-12-21 10:13 ` John Garry [this message]
2020-12-22 12:30   ` [PATCH 00/11] scsi: libsas: Remove in_interrupt() check Jason Yan
2020-12-22 12:54     ` John Garry
2021-01-11 13:43       ` Ahmed S. Darwish
2021-01-11 13:59         ` John Garry
2021-01-11 14:28           ` Ahmed S. Darwish

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9674052e-3deb-2a45-6082-4a40a472a219@huawei.com \
    --to=john.garry@huawei.com \
    --cc=a.darwish@linutronix.de \
    --cc=artur.paszkiewicz@intel.com \
    --cc=bigeasy@linutronix.de \
    --cc=dwagner@suse.de \
    --cc=hare@suse.com \
    --cc=jejb@linux.ibm.com \
    --cc=jinpu.wang@cloud.ionos.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=tglx@linutronix.de \
    --cc=yanaijie@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lkml.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git