From: wangyijing <wangyijing@huawei.com> To: Dan Williams <dan.j.williams@intel.com> Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>, "Martin K. Petersen" <martin.petersen@oracle.com>, <chenqilin2@huawei.com>, <hare@suse.com>, linux-scsi <linux-scsi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, <chenxiang66@hisilicon.com>, <huangdaode@hisilicon.com>, <wangkefeng.wang@huawei.com>, <zhaohongjiang@huawei.com>, <dingtianhong@huawei.com>, <guohanjun@huawei.com>, John Garry <john.garry@huawei.com>, Wei Fang <fangwei1@huawei.com>, <yanaijie@huawei.com>, Christoph Hellwig <hch@lst.de>, Yousong He <heyousong@huawei.com> Subject: Re: [PATCH 1/2] libsas: Don't process sas events in static works Date: Mon, 22 May 2017 13:54:30 +0800 [thread overview] Message-ID: <59227D16.6000102@huawei.com> (raw) In-Reply-To: <CAPcyv4idOqGpdFt=FKRPs0gAqYc8pOv+=gL3PGN0Wgp6maUcMw@mail.gmail.com> Hi Dan, thanks for your review and comments! 在 2017/5/21 11:44, Dan Williams 写道: > On Fri, May 19, 2017 at 11:39 PM, Yijing Wang <wangyijing@huawei.com> wrote: >> Now libsas hotplug work is static, LLDD driver queue >> the hotplug work into shost->work_q. If LLDD driver >> burst post lots hotplug events to libsas, the hotplug >> events may pending in the workqueue like >> >> shost->work_q >> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> processing >> |<-------wait worker to process-------->| >> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it >> to shost->work_q, but this work is already pending, so it would be lost. >> Finally, libsas delete the related sas port and sas devices, but LLDD driver >> expect libsas add the sas port and devices(last sas event). >> >> This patch remove the static defined hotplug work, and use dynamic work to >> avoid missing hotplug events. > > If we go this route we don't even need: > > sas_port_event_fns > sas_phy_event_fns > sas_ha_event_fns Yes, these three fns are not necessary, just for avoid lots kfree in phy/port/ha event fns. > > ...just specify the target routine directly to INIT_WORK() and remove > the indirection. > > I also think for safety this should use a mempool that guarantees that > events can continue to be processed under system memory pressure. What I am worried about is it's would still fail if the mempool is used empty during memory pressure. > Also, have you considered the case when a broken phy starts throwing a > constant stream of events? Is there a point at which libsas should > stop queuing events and disable the phy? Not yet, I didn't find this issue in real case, but I agree, it's really a problem in some broken hardware, I think it's not a easy problem, we could improve it step by step. Thanks! Yijing. > > . >
WARNING: multiple messages have this Message-ID (diff)
From: wangyijing <wangyijing@huawei.com> To: Dan Williams <dan.j.williams@intel.com> Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>, "Martin K. Petersen" <martin.petersen@oracle.com>, chenqilin2@huawei.com, hare@suse.com, linux-scsi <linux-scsi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, chenxiang66@hisilicon.com, huangdaode@hisilicon.com, wangkefeng.wang@huawei.com, zhaohongjiang@huawei.com, dingtianhong@huawei.com, guohanjun@huawei.com, John Garry <john.garry@huawei.com>, Wei Fang <fangwei1@huawei.com>, yanaijie@huawei.com, Christoph Hellwig <hch@lst.de>, Yousong He <heyousong@huawei.com> Subject: Re: [PATCH 1/2] libsas: Don't process sas events in static works Date: Mon, 22 May 2017 13:54:30 +0800 [thread overview] Message-ID: <59227D16.6000102@huawei.com> (raw) In-Reply-To: <CAPcyv4idOqGpdFt=FKRPs0gAqYc8pOv+=gL3PGN0Wgp6maUcMw@mail.gmail.com> Hi Dan, thanks for your review and comments! 在 2017/5/21 11:44, Dan Williams 写道: > On Fri, May 19, 2017 at 11:39 PM, Yijing Wang <wangyijing@huawei.com> wrote: >> Now libsas hotplug work is static, LLDD driver queue >> the hotplug work into shost->work_q. If LLDD driver >> burst post lots hotplug events to libsas, the hotplug >> events may pending in the workqueue like >> >> shost->work_q >> new work[PORTE_BYTES_DMAED] --> |[PHYE_LOSS_OF_SIGNAL][PORTE_BYTES_DMAED] -> processing >> |<-------wait worker to process-------->| >> In this case, a new PORTE_BYTES_DMAED event coming, libsas try to queue it >> to shost->work_q, but this work is already pending, so it would be lost. >> Finally, libsas delete the related sas port and sas devices, but LLDD driver >> expect libsas add the sas port and devices(last sas event). >> >> This patch remove the static defined hotplug work, and use dynamic work to >> avoid missing hotplug events. > > If we go this route we don't even need: > > sas_port_event_fns > sas_phy_event_fns > sas_ha_event_fns Yes, these three fns are not necessary, just for avoid lots kfree in phy/port/ha event fns. > > ...just specify the target routine directly to INIT_WORK() and remove > the indirection. > > I also think for safety this should use a mempool that guarantees that > events can continue to be processed under system memory pressure. What I am worried about is it's would still fail if the mempool is used empty during memory pressure. > Also, have you considered the case when a broken phy starts throwing a > constant stream of events? Is there a point at which libsas should > stop queuing events and disable the phy? Not yet, I didn't find this issue in real case, but I agree, it's really a problem in some broken hardware, I think it's not a easy problem, we could improve it step by step. Thanks! Yijing. > > . >
next prev parent reply other threads:[~2017-05-22 5:55 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-20 6:39 [PATCH 0/2] Enhance libsas hotplug feature Yijing Wang 2017-05-20 6:39 ` Yijing Wang 2017-05-20 6:39 ` [PATCH 1/2] libsas: Don't process sas events in static works Yijing Wang 2017-05-20 6:39 ` Yijing Wang 2017-05-21 3:44 ` Dan Williams 2017-05-22 5:54 ` wangyijing [this message] 2017-05-22 5:54 ` wangyijing 2017-05-22 9:28 ` John Garry 2017-05-22 9:28 ` John Garry 2017-05-23 6:39 ` wangyijing 2017-05-23 6:39 ` wangyijing 2017-05-20 6:39 ` [PATCH 2/2] libsas: Enhance libsas hotplug Yijing Wang 2017-05-20 6:39 ` Yijing Wang 2017-05-25 9:04 ` John Garry 2017-05-25 9:04 ` John Garry 2017-05-25 12:31 ` wangyijing 2017-05-25 12:31 ` wangyijing
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=59227D16.6000102@huawei.com \ --to=wangyijing@huawei.com \ --cc=chenqilin2@huawei.com \ --cc=chenxiang66@hisilicon.com \ --cc=dan.j.williams@intel.com \ --cc=dingtianhong@huawei.com \ --cc=fangwei1@huawei.com \ --cc=guohanjun@huawei.com \ --cc=hare@suse.com \ --cc=hch@lst.de \ --cc=heyousong@huawei.com \ --cc=huangdaode@hisilicon.com \ --cc=jejb@linux.vnet.ibm.com \ --cc=john.garry@huawei.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=wangkefeng.wang@huawei.com \ --cc=yanaijie@huawei.com \ --cc=zhaohongjiang@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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.