From: Peter Hilber <peter.hilber@opensynergy.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, vincent.guittot@linaro.org, souvik.chakravarty@arm.com, igor.skalkin@opensynergy.com, "Michael S. Tsirkin" <mst@redhat.com>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v9 09/11] firmware: arm_scmi: Add atomic mode support to virtio transport Date: Thu, 20 Jan 2022 20:09:56 +0100 [thread overview] Message-ID: <2f1ea794-a0b9-2099-edc0-b2aeb3ca6b92@opensynergy.com> (raw) In-Reply-To: <20220119122338.GE6113@e120937-lin> On 19.01.22 13:23, Cristian Marussi wrote: > On Tue, Jan 18, 2022 at 03:21:03PM +0100, Peter Hilber wrote: >> On 21.12.21 15:00, Cristian Marussi wrote: >>> Add support for .mark_txdone and .poll_done transport operations to SCMI >>> VirtIO transport as pre-requisites to enable atomic operations. >>> >>> Add a Kernel configuration option to enable SCMI VirtIO transport polling >>> and atomic mode for selected SCMI transactions while leaving it default >>> disabled. >>> >> >> Hi Cristian, >> >> thanks for the update. I have some more remarks inline below. >> > > Hi Peter, > > thanks for your review, much appreciated, please see my replies online. > >> My impression is that the virtio core does not expose helper functions suitable >> to busy-poll for used buffers. But changing this might not be difficult. Maybe >> more_used() from virtio_ring.c could be exposed via a wrapper? >> > > While I definitely agree that the virtio core support for polling is far from > ideal, some support is provided and my point was at first to try implement SCMI > virtio polling leveraging what we have now in the core and see if it was attainable > (indeed I tried early in this series to avoid as a whole to have to support polling > at the SCMI transport layer to attain SCMI cmds atomicity..but that was an ill > attempt that led nowhere good...) > > Btw, I was planning to post a new series next week (after merge-windows) with some > fixes I did already, at this point I'll include also some fixes derived > from some of your remarks. > >> Best regards, >> >> Peter >> [snip]>>> + * >>> + * Return: True once polling has successfully completed. >>> + */ >>> +static bool virtio_poll_done(struct scmi_chan_info *cinfo, >>> + struct scmi_xfer *xfer) >>> +{ >>> + bool pending, ret = false; >>> + unsigned int length, any_prefetched = 0; >>> + unsigned long flags; >>> + struct scmi_vio_msg *next_msg, *msg = xfer->priv; >>> + struct scmi_vio_channel *vioch = cinfo->transport_info; >>> + >>> + if (!msg) >>> + return true; >>> + >>> + spin_lock_irqsave(&msg->poll_lock, flags); >>> + /* Processed already by other polling loop on another CPU ? */ >>> + if (msg->poll_idx == VIO_MSG_POLL_DONE) { >>> + spin_unlock_irqrestore(&msg->poll_lock, flags); >>> + return true; >>> + } >>> + >>> + /* Has cmdq index moved at all ? */ >>> + pending = virtqueue_poll(vioch->vqueue, msg->poll_idx); >> >> In my understanding, the polling comparison could still be subject to the ABA >> problem when exactly 2**16 messages have been marked as used since >> msg->poll_idx was set (unlikely scenario, granted). >> >> I think this would be a lot simpler if the virtio core exported some >> concurrency-safe helper function for such polling (similar to more_used() from >> virtio_ring.c), as discussed at the top. > > So this is the main limitation indeed of the current implementation, I > cannot distinguish if there was an exact full wrap and I'm reading the same > last_idx as before but a whoppying 2**16 messages have instead gone through... > > The tricky part seems to me here that even introducing dedicated helpers > for polling in order to account for such wrapping (similar to more_used()) > those would be based by current VirtIO spec on a single bit wrap counter, > so how do you discern if 2 whole wraps have happened (even more unlikely..) ? > > Maybe I'm missing something though... > In my understanding, there is no need to keep track of the old state. We actually only want to check whether the device has marked any buffers as `used' which we did not retrieve yet via virtqueue_get_buf_ctx(). This is what more_used() checks in my understanding. One would just need to translate the external `struct virtqueue' param to the virtio_ring.c internal representation `struct vring_virtqueue' and then call `more_used()'. There would be no need to keep `poll_idx` then. Best regards, Peter > I'll have a though about this, but in my opinion this seems something so > unlikely that we could live with it, for the moment at least... > [snip]
WARNING: multiple messages have this Message-ID (diff)
From: Peter Hilber <peter.hilber@opensynergy.com> To: Cristian Marussi <cristian.marussi@arm.com> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, vincent.guittot@linaro.org, souvik.chakravarty@arm.com, igor.skalkin@opensynergy.com, "Michael S. Tsirkin" <mst@redhat.com>, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v9 09/11] firmware: arm_scmi: Add atomic mode support to virtio transport Date: Thu, 20 Jan 2022 20:09:56 +0100 [thread overview] Message-ID: <2f1ea794-a0b9-2099-edc0-b2aeb3ca6b92@opensynergy.com> (raw) In-Reply-To: <20220119122338.GE6113@e120937-lin> On 19.01.22 13:23, Cristian Marussi wrote: > On Tue, Jan 18, 2022 at 03:21:03PM +0100, Peter Hilber wrote: >> On 21.12.21 15:00, Cristian Marussi wrote: >>> Add support for .mark_txdone and .poll_done transport operations to SCMI >>> VirtIO transport as pre-requisites to enable atomic operations. >>> >>> Add a Kernel configuration option to enable SCMI VirtIO transport polling >>> and atomic mode for selected SCMI transactions while leaving it default >>> disabled. >>> >> >> Hi Cristian, >> >> thanks for the update. I have some more remarks inline below. >> > > Hi Peter, > > thanks for your review, much appreciated, please see my replies online. > >> My impression is that the virtio core does not expose helper functions suitable >> to busy-poll for used buffers. But changing this might not be difficult. Maybe >> more_used() from virtio_ring.c could be exposed via a wrapper? >> > > While I definitely agree that the virtio core support for polling is far from > ideal, some support is provided and my point was at first to try implement SCMI > virtio polling leveraging what we have now in the core and see if it was attainable > (indeed I tried early in this series to avoid as a whole to have to support polling > at the SCMI transport layer to attain SCMI cmds atomicity..but that was an ill > attempt that led nowhere good...) > > Btw, I was planning to post a new series next week (after merge-windows) with some > fixes I did already, at this point I'll include also some fixes derived > from some of your remarks. > >> Best regards, >> >> Peter >> [snip]>>> + * >>> + * Return: True once polling has successfully completed. >>> + */ >>> +static bool virtio_poll_done(struct scmi_chan_info *cinfo, >>> + struct scmi_xfer *xfer) >>> +{ >>> + bool pending, ret = false; >>> + unsigned int length, any_prefetched = 0; >>> + unsigned long flags; >>> + struct scmi_vio_msg *next_msg, *msg = xfer->priv; >>> + struct scmi_vio_channel *vioch = cinfo->transport_info; >>> + >>> + if (!msg) >>> + return true; >>> + >>> + spin_lock_irqsave(&msg->poll_lock, flags); >>> + /* Processed already by other polling loop on another CPU ? */ >>> + if (msg->poll_idx == VIO_MSG_POLL_DONE) { >>> + spin_unlock_irqrestore(&msg->poll_lock, flags); >>> + return true; >>> + } >>> + >>> + /* Has cmdq index moved at all ? */ >>> + pending = virtqueue_poll(vioch->vqueue, msg->poll_idx); >> >> In my understanding, the polling comparison could still be subject to the ABA >> problem when exactly 2**16 messages have been marked as used since >> msg->poll_idx was set (unlikely scenario, granted). >> >> I think this would be a lot simpler if the virtio core exported some >> concurrency-safe helper function for such polling (similar to more_used() from >> virtio_ring.c), as discussed at the top. > > So this is the main limitation indeed of the current implementation, I > cannot distinguish if there was an exact full wrap and I'm reading the same > last_idx as before but a whoppying 2**16 messages have instead gone through... > > The tricky part seems to me here that even introducing dedicated helpers > for polling in order to account for such wrapping (similar to more_used()) > those would be based by current VirtIO spec on a single bit wrap counter, > so how do you discern if 2 whole wraps have happened (even more unlikely..) ? > > Maybe I'm missing something though... > In my understanding, there is no need to keep track of the old state. We actually only want to check whether the device has marked any buffers as `used' which we did not retrieve yet via virtqueue_get_buf_ctx(). This is what more_used() checks in my understanding. One would just need to translate the external `struct virtqueue' param to the virtio_ring.c internal representation `struct vring_virtqueue' and then call `more_used()'. There would be no need to keep `poll_idx` then. Best regards, Peter > I'll have a though about this, but in my opinion this seems something so > unlikely that we could live with it, for the moment at least... > [snip] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-01-20 19:10 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-20 19:56 [PATCH v8 00/11] Introduce atomic support for SCMI transports Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 01/11] firmware: arm_scmi: Add configurable polling mode for transports Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-21 19:38 ` Pratyush Yadav 2021-12-21 19:38 ` Pratyush Yadav 2021-12-21 20:23 ` Sudeep Holla 2021-12-21 20:23 ` Sudeep Holla 2021-12-20 19:56 ` [PATCH v8 02/11] firmware: arm_scmi: Make smc transport use common completions Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 03/11] firmware: arm_scmi: Add sync_cmds_completed_on_ret transport flag Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 04/11] firmware: arm_scmi: Make smc support sync_cmds_completed_on_ret Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 05/11] firmware: arm_scmi: Make optee " Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 06/11] firmware: arm_scmi: Add support for atomic transports Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 07/11] firmware: arm_scmi: Add atomic mode support to smc transport Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 08/11] firmware: arm_scmi: Add new parameter to mark_txdone Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 09/11] firmware: arm_scmi: Add atomic mode support to virtio transport Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 23:17 ` Michael S. Tsirkin 2021-12-20 23:17 ` Michael S. Tsirkin 2021-12-20 23:17 ` Michael S. Tsirkin 2021-12-21 12:09 ` Cristian Marussi 2021-12-21 12:09 ` Cristian Marussi 2021-12-21 14:00 ` [PATCH v9 " Cristian Marussi 2021-12-21 14:00 ` Cristian Marussi 2022-01-18 14:21 ` Peter Hilber 2022-01-18 14:21 ` Peter Hilber 2022-01-19 12:23 ` Cristian Marussi 2022-01-19 12:23 ` Cristian Marussi 2022-01-20 19:09 ` Peter Hilber [this message] 2022-01-20 19:09 ` Peter Hilber 2022-01-20 20:39 ` Michael S. Tsirkin 2022-01-20 20:39 ` Michael S. Tsirkin 2022-01-20 20:39 ` Michael S. Tsirkin 2022-01-23 20:02 ` Cristian Marussi 2022-01-23 20:02 ` Cristian Marussi 2022-01-23 22:40 ` Michael S. Tsirkin 2022-01-23 22:40 ` Michael S. Tsirkin 2022-01-23 22:40 ` Michael S. Tsirkin 2022-01-23 22:45 ` Cristian Marussi 2022-01-23 22:45 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 10/11] firmware: arm_scmi: Add atomic support to clock protocol Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2021-12-20 19:56 ` [PATCH v8 11/11] clk: scmi: Support atomic clock enable/disable API Cristian Marussi 2021-12-20 19:56 ` Cristian Marussi 2022-01-14 23:08 ` Stephen Boyd 2022-01-14 23:08 ` Stephen Boyd 2022-01-17 10:31 ` Sudeep Holla 2022-01-17 10:31 ` Sudeep Holla 2022-01-17 12:40 ` Cristian Marussi 2022-01-17 12:40 ` Cristian Marussi 2021-12-22 14:23 ` [PATCH v8 00/11] (subset) Introduce atomic support for SCMI transports Sudeep Holla 2021-12-22 14:23 ` Sudeep Holla 2022-01-11 18:13 ` [PATCH v8 00/11] " Cristian Marussi 2022-01-11 18:13 ` Cristian Marussi
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=2f1ea794-a0b9-2099-edc0-b2aeb3ca6b92@opensynergy.com \ --to=peter.hilber@opensynergy.com \ --cc=Jonathan.Cameron@Huawei.com \ --cc=cristian.marussi@arm.com \ --cc=etienne.carriere@linaro.org \ --cc=f.fainelli@gmail.com \ --cc=igor.skalkin@opensynergy.com \ --cc=james.quinlan@broadcom.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=souvik.chakravarty@arm.com \ --cc=sudeep.holla@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=virtualization@lists.linux-foundation.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.