Messages in this thread | | | Date | Thu, 20 Jan 2022 20:09:56 +0100 | From | Peter Hilber <> | Subject | Re: [PATCH v9 09/11] firmware: arm_scmi: Add atomic mode support to virtio transport |
| |
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]
| |