From: Alex Elder <elder@linaro.org> To: Arnd Bergmann <arnd@arndb.de> Cc: David Miller <davem@davemloft.net>, Bjorn Andersson <bjorn.andersson@linaro.org>, Ilias Apalodimas <ilias.apalodimas@linaro.org>, Networking <netdev@vger.kernel.org>, DTML <devicetree@vger.kernel.org>, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, syadagir@codeaurora.org, mjavid@codeaurora.org, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com> Subject: Re: [RFC PATCH 10/12] soc: qcom: ipa: data path Date: Wed, 14 Nov 2018 21:31:12 -0600 [thread overview] Message-ID: <a1517ea0-b651-f24e-4890-8ceb6e2b14c6@linaro.org> (raw) In-Reply-To: <CAK8P3a0nkfwFCCVHaTJ+kJGWxO+qFTzTLnRgB-NG0AyMEsv3bA@mail.gmail.com> On 11/7/18 8:55 AM, Arnd Bergmann wrote: > On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote: >> >> This patch contains "ipa_dp.c", which includes the bulk of the data >> path code. There is an overview in the code of how things operate, >> but there are already plans to rework this portion of the driver. >> >> In particular: >> - Interrupt handling will be replaced with a threaded interrupt >> handler. Currently handling occurs in a combination of >> interrupt and workqueue context, and this requires locking >> and atomic operations for proper synchronization. > > You probably don't want to use just a threaded IRQ handler to > start the poll function, that would still require an extra indirection. That's a really good point. However I think that the path I'll take to *getting* to scheduling the poll in interrupt context will use a threaded interrupt handler. I'm hoping that will allow me to simplify the code in steps. The main reason for this split between working in interrupt context when possible, but pushing to a workqueue when not, is to allow IPA clock(s) to be turned off. Enabling the clocks is a blocking operation, so can't' be done in the top half interrupt handler. The thought was it would be best to work in interrupt context--if the clock was already active--but to defer to a workqueue to turn the clock on if necessary. The result requires locking and duplication of code that I find to be pretty confusing--and hard to reason about. I have been planning to re-do things to be better suited to NAPI, and knowing that, I haven't given the data path as much attention as some of the rest. > However, you can probably use the top half of the threaded > handler to request the poll function if necessary but use > the bottom half for anything that does not go through poll. > >> - Currently, only receive endpoints use NAPI. Transmit >> completion interrupts are disabled, and are handled in batches >> by periodically scheduling an interrupting no-op request. >> The plan is to arrange for transmit requests to generate >> interrupts, and their completion will be processed with other >> completions in the NAPI poll function. This will also allow >> accurate feedback about packet sojourn time to be provided to >> queue limiting mechanisms. > > Right, that is definitely required here. I also had a look at > the gsi_channel_queue() function, which sits in the middle of > the transmit function and is rather unoptimized. I'd suggest moving > that into the caller so we can see what is going on, and then > optimizing it from there. Yes, I agree with that. There are multiple levels of abstraction in play and they aren't helpful. We have ipa_desc structures that are translated by ipa_send() into gsi_xfer_elem structures, which are ultimately recorded by gsi_channel_queue() as 16 byte gsi_tre structures. At least one of those translations can go away. >> - Not all receive endpoints use NAPI. The plan is for *all* >> endpoints to use NAPI. And because all endpoints share a >> common GSI interrupt, a single NAPI structure will used to >> managing the processing for all completions on all endpoints. >> - Receive buffers are posted to the hardware by a workqueue >> function. Instead, the plan is to have this done by the >> NAPI poll routine. > > Makes sense, yes. Thanks. -Alex > > Arnd >
WARNING: multiple messages have this Message-ID (diff)
From: elder@linaro.org (Alex Elder) To: linux-arm-kernel@lists.infradead.org Subject: [RFC PATCH 10/12] soc: qcom: ipa: data path Date: Wed, 14 Nov 2018 21:31:12 -0600 [thread overview] Message-ID: <a1517ea0-b651-f24e-4890-8ceb6e2b14c6@linaro.org> (raw) In-Reply-To: <CAK8P3a0nkfwFCCVHaTJ+kJGWxO+qFTzTLnRgB-NG0AyMEsv3bA@mail.gmail.com> On 11/7/18 8:55 AM, Arnd Bergmann wrote: > On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote: >> >> This patch contains "ipa_dp.c", which includes the bulk of the data >> path code. There is an overview in the code of how things operate, >> but there are already plans to rework this portion of the driver. >> >> In particular: >> - Interrupt handling will be replaced with a threaded interrupt >> handler. Currently handling occurs in a combination of >> interrupt and workqueue context, and this requires locking >> and atomic operations for proper synchronization. > > You probably don't want to use just a threaded IRQ handler to > start the poll function, that would still require an extra indirection. That's a really good point. However I think that the path I'll take to *getting* to scheduling the poll in interrupt context will use a threaded interrupt handler. I'm hoping that will allow me to simplify the code in steps. The main reason for this split between working in interrupt context when possible, but pushing to a workqueue when not, is to allow IPA clock(s) to be turned off. Enabling the clocks is a blocking operation, so can't' be done in the top half interrupt handler. The thought was it would be best to work in interrupt context--if the clock was already active--but to defer to a workqueue to turn the clock on if necessary. The result requires locking and duplication of code that I find to be pretty confusing--and hard to reason about. I have been planning to re-do things to be better suited to NAPI, and knowing that, I haven't given the data path as much attention as some of the rest. > However, you can probably use the top half of the threaded > handler to request the poll function if necessary but use > the bottom half for anything that does not go through poll. > >> - Currently, only receive endpoints use NAPI. Transmit >> completion interrupts are disabled, and are handled in batches >> by periodically scheduling an interrupting no-op request. >> The plan is to arrange for transmit requests to generate >> interrupts, and their completion will be processed with other >> completions in the NAPI poll function. This will also allow >> accurate feedback about packet sojourn time to be provided to >> queue limiting mechanisms. > > Right, that is definitely required here. I also had a look at > the gsi_channel_queue() function, which sits in the middle of > the transmit function and is rather unoptimized. I'd suggest moving > that into the caller so we can see what is going on, and then > optimizing it from there. Yes, I agree with that. There are multiple levels of abstraction in play and they aren't helpful. We have ipa_desc structures that are translated by ipa_send() into gsi_xfer_elem structures, which are ultimately recorded by gsi_channel_queue() as 16 byte gsi_tre structures. At least one of those translations can go away. >> - Not all receive endpoints use NAPI. The plan is for *all* >> endpoints to use NAPI. And because all endpoints share a >> common GSI interrupt, a single NAPI structure will used to >> managing the processing for all completions on all endpoints. >> - Receive buffers are posted to the hardware by a workqueue >> function. Instead, the plan is to have this done by the >> NAPI poll routine. > > Makes sense, yes. Thanks. -Alex > > Arnd >
next prev parent reply other threads:[~2018-11-15 3:31 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-07 0:32 [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 01/12] dt-bindings: soc: qcom: add IPA bindings Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 11:50 ` Arnd Bergmann 2018-11-07 11:50 ` Arnd Bergmann 2018-11-09 22:38 ` Alex Elder 2018-11-09 22:38 ` Alex Elder 2018-11-07 14:59 ` Rob Herring 2018-11-07 14:59 ` Rob Herring 2018-11-09 22:38 ` Alex Elder 2018-11-09 22:38 ` Alex Elder 2018-11-11 1:40 ` Rob Herring 2018-11-11 1:40 ` Rob Herring 2018-11-11 1:40 ` Rob Herring 2018-11-13 16:28 ` Alex Elder 2018-11-13 16:28 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 02/12] soc: qcom: ipa: DMA helpers Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 12:17 ` Arnd Bergmann 2018-11-07 12:17 ` Arnd Bergmann 2018-11-13 16:33 ` Alex Elder 2018-11-13 16:33 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 03/12] soc: qcom: ipa: generic software interface Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 04/12] soc: qcom: ipa: immediate commands Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:36 ` Arnd Bergmann 2018-11-07 14:36 ` Arnd Bergmann 2018-11-13 16:58 ` Alex Elder 2018-11-13 16:58 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 05/12] soc: qcom: ipa: IPA interrupts and the microcontroller Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 06/12] soc: qcom: ipa: QMI modem communication Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 07/12] soc: qcom: ipa: IPA register abstraction Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 15:00 ` Arnd Bergmann 2018-11-07 15:00 ` Arnd Bergmann 2018-11-15 2:48 ` Alex Elder 2018-11-15 2:48 ` Alex Elder 2018-11-15 14:42 ` Arnd Bergmann 2018-11-15 14:42 ` Arnd Bergmann 2018-11-07 0:32 ` [RFC PATCH 08/12] soc: qcom: ipa: utility functions Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:08 ` Arnd Bergmann 2018-11-07 14:08 ` Arnd Bergmann 2018-11-15 3:11 ` Alex Elder 2018-11-15 3:11 ` Alex Elder 2018-11-07 0:32 ` [RFC PATCH 10/12] soc: qcom: ipa: data path Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 14:55 ` Arnd Bergmann 2018-11-07 14:55 ` Arnd Bergmann 2018-11-15 3:31 ` Alex Elder [this message] 2018-11-15 3:31 ` Alex Elder 2018-11-15 14:48 ` Arnd Bergmann 2018-11-15 14:48 ` Arnd Bergmann 2018-11-07 0:32 ` [RFC PATCH 11/12] soc: qcom: ipa: IPA rmnet interface Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 13:30 ` Arnd Bergmann 2018-11-07 13:30 ` Arnd Bergmann 2018-11-07 15:26 ` Dan Williams 2018-11-07 15:26 ` Dan Williams 2018-11-07 0:32 ` [RFC PATCH 12/12] soc: qcom: ipa: build and "ipa_i.h" Alex Elder 2018-11-07 0:32 ` Alex Elder 2018-11-07 0:40 ` Randy Dunlap 2018-11-07 0:40 ` Randy Dunlap 2018-11-07 0:40 ` Randy Dunlap 2018-11-08 16:22 ` Alex Elder 2018-11-08 16:22 ` Alex Elder 2018-11-07 12:34 ` Arnd Bergmann 2018-11-07 12:34 ` Arnd Bergmann 2018-11-07 15:46 ` [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Arnd Bergmann 2018-11-07 15:46 ` Arnd Bergmann
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=a1517ea0-b651-f24e-4890-8ceb6e2b14c6@linaro.org \ --to=elder@linaro.org \ --cc=arnd@arndb.de \ --cc=bjorn.andersson@linaro.org \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=ilias.apalodimas@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-soc@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mjavid@codeaurora.org \ --cc=netdev@vger.kernel.org \ --cc=robh+dt@kernel.org \ --cc=syadagir@codeaurora.org \ /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.