| Message ID | 20201218204354.586951-1-a.darwish@linutronix.de |
|---|---|
| Headers | show |
| Series | scsi: libsas: Remove in_interrupt() check | expand |
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 > . >
在 2020/12/21 18:13, 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@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 The default value is 1024. > 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. > Pre-allocated memory is an option.(Which we have tried at the very beginnig by Wang Yijing.) Or directly use GFP_ATOMIC is maybe better than passing flags from lldds. Thanks, Jason > 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 >> . >> > > .
On 22/12/2020 12:30, Jason Yan wrote: >> return event; >> >> >> So default for phy->ha->event_thres is 32, and I can't imagine that > > The default value is 1024. Ah, 32 is the minimum allowed set via sysfs. > >> 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. >> > > Pre-allocated memory is an option.(Which we have tried at the very > beginnig by Wang Yijing.) Right, I remember this, but I think the concern was having a proper method to manage this pre-allocated memory then. And same problem now. > > Or directly use GFP_ATOMIC is maybe better than passing flags from lldds. > I think that if we don't really need this, then should not use it. Thanks, John
Hi John, Jason, On Tue, Dec 22, 2020 at 12:54:58PM +0000, John Garry wrote: > On 22/12/2020 12:30, Jason Yan wrote: > > > return event; > > > > > > > > > So default for phy->ha->event_thres is 32, and I can't imagine that > > > > The default value is 1024. > > Ah, 32 is the minimum allowed set via sysfs. > > > > > > 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. > > > > > > > Pre-allocated memory is an option.(Which we have tried at the very > > beginnig by Wang Yijing.) > > Right, I remember this, but I think the concern was having a proper method > to manage this pre-allocated memory then. And same problem now. > > > > > Or directly use GFP_ATOMIC is maybe better than passing flags from lldds. > > > > I think that if we don't really need this, then should not use it. > Kind reminder. Do we have any consensus here? Thanks, -- Ahmed S. Darwish Linutronix GmbH
On 11/01/2021 13:43, Ahmed S. Darwish wrote: > Hi John, Jason, > > On Tue, Dec 22, 2020 at 12:54:58PM +0000, John Garry wrote: >> On 22/12/2020 12:30, Jason Yan wrote: >>>> return event; >>>> >>>> >>>> So default for phy->ha->event_thres is 32, and I can't imagine that >>> The default value is 1024. >> Ah, 32 is the minimum allowed set via sysfs. >> >>>> 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. >>>> >>> Pre-allocated memory is an option.(Which we have tried at the very >>> beginnig by Wang Yijing.) >> Right, I remember this, but I think the concern was having a proper method >> to manage this pre-allocated memory then. And same problem now. >> >>> Or directly use GFP_ATOMIC is maybe better than passing flags from lldds. >>> >> I think that if we don't really need this, then should not use it. >> > Kind reminder. Do we have any consensus here? > Hi Ahmed, To me, what you're doing seems fine. I was looking for some API to manage small memory pools and which is atomic safe to avoid passing the context flag, but I didn't find such a thing. Just one other thing to mention: I have a patch to remove the indirection in libsas notifiers: https://github.com/hisilicon/kernel-dev/commit/87fcd7e113dc05b7933260e7fa4588dc3730cc2a I was going to send it today. Hopefully, if community has no problem with it, you can make your changes with that in mind. Thanks, John
On Mon, Jan 11, 2021 at 01:59:25PM +0000, John Garry wrote: ... > To me, what you're doing seems fine. > ... > > Just one other thing to mention: > I have a patch to remove the indirection in libsas notifiers: > https://github.com/hisilicon/kernel-dev/commit/87fcd7e113dc05b7933260e7fa4588dc3730cc2a > > I was going to send it today. Hopefully, if community has no problem with > it, you can make your changes with that in mind. > Perfect. I'll rebase on top of it if everything is OK there. I'll also append some patches to the series, removing the _gfp() suffix, per your request earlier: https://lkml.kernel.org/r/68957d37-c789-0f0e-f5d1-85fef7f39f4f@huawei.com Thanks! -- Ahmed S. Darwish Linutronix GmbH
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); ... } 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