From: Cristian Marussi <cristian.marussi@arm.com> To: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Cc: 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, peter.hilber@opensynergy.com, igor.skalkin@opensynergy.com, cristian.marussi@arm.com, "Michael S. Tsirkin" <mst@redhat.com>, virtualization@lists.linux-foundation.org Subject: [PATCH v2 3/9] [RFC] virtio_ring: Embed a wrap counter in opaque poll index value Date: Tue, 1 Feb 2022 17:15:55 +0000 [thread overview] Message-ID: <20220201171601.53316-4-cristian.marussi@arm.com> (raw) In-Reply-To: <20220201171601.53316-1-cristian.marussi@arm.com> Exported API virtqueue_poll() can be used to support polling mode operation on top of virtio layer if needed; currently the parameter last_used_idx is the opaque value that needs to be passed to the virtqueue_poll() function to check if there are new pending used buffers in the queue: such opaque value would have been previously obtained by a call to the API function virtqueue_enable_cb_prepare(). Since such opaque value is indeed containing simply a snapshot in time of the internal last_used_index (roughly), it is possible that, if exactly 2**16 buffers are marked as used between two successive calls to virtqueue_poll(), the caller is fooled into thinking that nothing is pending (ABA problem). Keep a full fledged internal wraps counter per virtqueue and embed it into the upper 16bits of the returned opaque value, so that the above scenario can be detected transparently by virtqueue_poll(): this way each single possible last_used_idx value is really belonging to a different wrap. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Igor Skalkin <igor.skalkin@opensynergy.com> Cc: Peter Hilber <peter.hilber@opensynergy.com> Cc: virtualization@lists.linux-foundation.org Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- Still no perf data on this, I was wondering what exactly to measure in term of perf metrics to evaluate the impact of the rolling vq->wraps counter. --- drivers/virtio/virtio_ring.c | 51 +++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 00f64f2f8b72..613ec0503509 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -12,6 +12,8 @@ #include <linux/hrtimer.h> #include <linux/dma-mapping.h> #include <linux/spinlock.h> +#include <linux/bits.h> +#include <linux/bitfield.h> #include <xen/xen.h> static bool force_used_validation = false; @@ -69,6 +71,17 @@ module_param(force_used_validation, bool, 0444); #define LAST_ADD_TIME_INVALID(vq) #endif +#define VRING_IDX_MASK GENMASK(15, 0) +#define VRING_GET_IDX(opaque) \ + ((u16)FIELD_GET(VRING_IDX_MASK, (opaque))) + +#define VRING_WRAPS_MASK GENMASK(31, 16) +#define VRING_GET_WRAPS(opaque) \ + ((u16)FIELD_GET(VRING_WRAPS_MASK, (opaque))) + +#define VRING_BUILD_OPAQUE(idx, wraps) \ + (FIELD_PREP(VRING_WRAPS_MASK, (wraps)) | ((idx) & VRING_IDX_MASK)) + struct vring_desc_state_split { void *data; /* Data for callback. */ struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ @@ -117,6 +130,8 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; + u16 wraps; + /* Hint for event idx: already triggered no need to disable. */ bool event_triggered; @@ -806,6 +821,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, ret = vq->split.desc_state[i].data; detach_buf_split(vq, i, ctx); vq->last_used_idx++; + if (unlikely(!vq->last_used_idx)) + vq->wraps++; /* If we expect an interrupt for the next entry, tell host * by writing event index and flush out the write before * the read in the next get_buf call. */ @@ -1508,6 +1525,7 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) { vq->last_used_idx -= vq->packed.vring.num; vq->packed.used_wrap_counter ^= 1; + vq->wraps++; } /* @@ -1744,6 +1762,7 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->wraps = 0; vq->event_triggered = false; vq->num_added = 0; vq->packed_ring = true; @@ -2092,13 +2111,17 @@ EXPORT_SYMBOL_GPL(virtqueue_disable_cb); */ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq) { + unsigned int last_used_idx; struct vring_virtqueue *vq = to_vvq(_vq); if (vq->event_triggered) vq->event_triggered = false; - return vq->packed_ring ? virtqueue_enable_cb_prepare_packed(_vq) : - virtqueue_enable_cb_prepare_split(_vq); + last_used_idx = vq->packed_ring ? + virtqueue_enable_cb_prepare_packed(_vq) : + virtqueue_enable_cb_prepare_split(_vq); + + return VRING_BUILD_OPAQUE(last_used_idx, vq->wraps); } EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare); @@ -2107,6 +2130,21 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare); * @_vq: the struct virtqueue we're talking about. * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare). * + * The provided last_used_idx, as returned by virtqueue_enable_cb_prepare(), + * is an opaque value representing the queue state and it is built as follows: + * + * --------------------------------------------------------- + * | vq->wraps | vq->last_used_idx | + * 31------------------------------------------------------0 + * + * The MSB 16bits embedding the wraps counter for the underlying virtqueue + * is stripped out here before reaching into the lower layer helpers. + * + * This structure of the opaque value mitigates the scenario in which, when + * exactly 2**16 messages are marked as used between two successive calls to + * virtqueue_poll(), the caller is fooled into thinking nothing new has arrived + * since the pure last_used_idx is exactly the same. + * * Returns "true" if there are pending used buffers in the queue. * * This does not need to be serialized. @@ -2118,9 +2156,13 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx) if (unlikely(vq->broken)) return false; + if (unlikely(vq->wraps != VRING_GET_WRAPS(last_used_idx))) + return true; + virtio_mb(vq->weak_barriers); - return vq->packed_ring ? virtqueue_poll_packed(_vq, last_used_idx) : - virtqueue_poll_split(_vq, last_used_idx); + return vq->packed_ring ? + virtqueue_poll_packed(_vq, VRING_GET_IDX(last_used_idx)) : + virtqueue_poll_split(_vq, VRING_GET_IDX(last_used_idx)); } EXPORT_SYMBOL_GPL(virtqueue_poll); @@ -2245,6 +2287,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->wraps = 0; vq->event_triggered = false; vq->num_added = 0; vq->use_dma_api = vring_use_dma_api(vdev); -- 2.17.1
WARNING: multiple messages have this Message-ID (diff)
From: Cristian Marussi <cristian.marussi@arm.com> To: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Cc: 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, peter.hilber@opensynergy.com, igor.skalkin@opensynergy.com, cristian.marussi@arm.com, "Michael S. Tsirkin" <mst@redhat.com>, virtualization@lists.linux-foundation.org Subject: [PATCH v2 3/9] [RFC] virtio_ring: Embed a wrap counter in opaque poll index value Date: Tue, 1 Feb 2022 17:15:55 +0000 [thread overview] Message-ID: <20220201171601.53316-4-cristian.marussi@arm.com> (raw) In-Reply-To: <20220201171601.53316-1-cristian.marussi@arm.com> Exported API virtqueue_poll() can be used to support polling mode operation on top of virtio layer if needed; currently the parameter last_used_idx is the opaque value that needs to be passed to the virtqueue_poll() function to check if there are new pending used buffers in the queue: such opaque value would have been previously obtained by a call to the API function virtqueue_enable_cb_prepare(). Since such opaque value is indeed containing simply a snapshot in time of the internal last_used_index (roughly), it is possible that, if exactly 2**16 buffers are marked as used between two successive calls to virtqueue_poll(), the caller is fooled into thinking that nothing is pending (ABA problem). Keep a full fledged internal wraps counter per virtqueue and embed it into the upper 16bits of the returned opaque value, so that the above scenario can be detected transparently by virtqueue_poll(): this way each single possible last_used_idx value is really belonging to a different wrap. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Igor Skalkin <igor.skalkin@opensynergy.com> Cc: Peter Hilber <peter.hilber@opensynergy.com> Cc: virtualization@lists.linux-foundation.org Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- Still no perf data on this, I was wondering what exactly to measure in term of perf metrics to evaluate the impact of the rolling vq->wraps counter. --- drivers/virtio/virtio_ring.c | 51 +++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 00f64f2f8b72..613ec0503509 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -12,6 +12,8 @@ #include <linux/hrtimer.h> #include <linux/dma-mapping.h> #include <linux/spinlock.h> +#include <linux/bits.h> +#include <linux/bitfield.h> #include <xen/xen.h> static bool force_used_validation = false; @@ -69,6 +71,17 @@ module_param(force_used_validation, bool, 0444); #define LAST_ADD_TIME_INVALID(vq) #endif +#define VRING_IDX_MASK GENMASK(15, 0) +#define VRING_GET_IDX(opaque) \ + ((u16)FIELD_GET(VRING_IDX_MASK, (opaque))) + +#define VRING_WRAPS_MASK GENMASK(31, 16) +#define VRING_GET_WRAPS(opaque) \ + ((u16)FIELD_GET(VRING_WRAPS_MASK, (opaque))) + +#define VRING_BUILD_OPAQUE(idx, wraps) \ + (FIELD_PREP(VRING_WRAPS_MASK, (wraps)) | ((idx) & VRING_IDX_MASK)) + struct vring_desc_state_split { void *data; /* Data for callback. */ struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ @@ -117,6 +130,8 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; + u16 wraps; + /* Hint for event idx: already triggered no need to disable. */ bool event_triggered; @@ -806,6 +821,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, ret = vq->split.desc_state[i].data; detach_buf_split(vq, i, ctx); vq->last_used_idx++; + if (unlikely(!vq->last_used_idx)) + vq->wraps++; /* If we expect an interrupt for the next entry, tell host * by writing event index and flush out the write before * the read in the next get_buf call. */ @@ -1508,6 +1525,7 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) { vq->last_used_idx -= vq->packed.vring.num; vq->packed.used_wrap_counter ^= 1; + vq->wraps++; } /* @@ -1744,6 +1762,7 @@ static struct virtqueue *vring_create_virtqueue_packed( vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->wraps = 0; vq->event_triggered = false; vq->num_added = 0; vq->packed_ring = true; @@ -2092,13 +2111,17 @@ EXPORT_SYMBOL_GPL(virtqueue_disable_cb); */ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq) { + unsigned int last_used_idx; struct vring_virtqueue *vq = to_vvq(_vq); if (vq->event_triggered) vq->event_triggered = false; - return vq->packed_ring ? virtqueue_enable_cb_prepare_packed(_vq) : - virtqueue_enable_cb_prepare_split(_vq); + last_used_idx = vq->packed_ring ? + virtqueue_enable_cb_prepare_packed(_vq) : + virtqueue_enable_cb_prepare_split(_vq); + + return VRING_BUILD_OPAQUE(last_used_idx, vq->wraps); } EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare); @@ -2107,6 +2130,21 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb_prepare); * @_vq: the struct virtqueue we're talking about. * @last_used_idx: virtqueue state (from call to virtqueue_enable_cb_prepare). * + * The provided last_used_idx, as returned by virtqueue_enable_cb_prepare(), + * is an opaque value representing the queue state and it is built as follows: + * + * --------------------------------------------------------- + * | vq->wraps | vq->last_used_idx | + * 31------------------------------------------------------0 + * + * The MSB 16bits embedding the wraps counter for the underlying virtqueue + * is stripped out here before reaching into the lower layer helpers. + * + * This structure of the opaque value mitigates the scenario in which, when + * exactly 2**16 messages are marked as used between two successive calls to + * virtqueue_poll(), the caller is fooled into thinking nothing new has arrived + * since the pure last_used_idx is exactly the same. + * * Returns "true" if there are pending used buffers in the queue. * * This does not need to be serialized. @@ -2118,9 +2156,13 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned last_used_idx) if (unlikely(vq->broken)) return false; + if (unlikely(vq->wraps != VRING_GET_WRAPS(last_used_idx))) + return true; + virtio_mb(vq->weak_barriers); - return vq->packed_ring ? virtqueue_poll_packed(_vq, last_used_idx) : - virtqueue_poll_split(_vq, last_used_idx); + return vq->packed_ring ? + virtqueue_poll_packed(_vq, VRING_GET_IDX(last_used_idx)) : + virtqueue_poll_split(_vq, VRING_GET_IDX(last_used_idx)); } EXPORT_SYMBOL_GPL(virtqueue_poll); @@ -2245,6 +2287,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index, vq->weak_barriers = weak_barriers; vq->broken = false; vq->last_used_idx = 0; + vq->wraps = 0; vq->event_triggered = false; vq->num_added = 0; vq->use_dma_api = vring_use_dma_api(vdev); -- 2.17.1 _______________________________________________ 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-02-01 17:16 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-01 17:15 [PATCH 0/9] Add SCMI Virtio & Clock atomic support Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 1/9] firmware: arm_scmi: Add a virtio channel refcount Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 22:05 ` Michael S. Tsirkin 2022-02-01 22:05 ` Michael S. Tsirkin 2022-02-03 10:08 ` Cristian Marussi 2022-02-03 10:08 ` Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 2/9] firmware: arm_scmi: Review virtio free_list handling Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi [this message] 2022-02-01 17:15 ` [PATCH v2 3/9] [RFC] virtio_ring: Embed a wrap counter in opaque poll index value Cristian Marussi 2022-02-01 18:27 ` Michael S. Tsirkin 2022-02-01 18:27 ` Michael S. Tsirkin 2022-02-01 18:27 ` Michael S. Tsirkin 2022-02-03 10:51 ` Cristian Marussi 2022-02-03 10:51 ` Cristian Marussi 2022-02-03 11:32 ` Michael S. Tsirkin 2022-02-03 11:32 ` Michael S. Tsirkin 2022-02-03 11:32 ` Michael S. Tsirkin 2022-02-07 18:52 ` Cristian Marussi 2022-02-07 18:52 ` Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 4/9] firmware: arm_scmi: Add atomic mode support to virtio transport Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 5/9] dt-bindings: firmware: arm,scmi: Add atomic_threshold optional property Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 5/9] dt-bindings: firmware: arm, scmi: " Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 6/9] firmware: arm_scmi: Support optional system wide atomic_threshold Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 17:15 ` [PATCH v2 7/9] firmware: arm_scmi: Add atomic support to clock protocol Cristian Marussi 2022-02-01 17:15 ` Cristian Marussi 2022-02-01 17:16 ` [PATCH v2 8/9] [RFC] firmware: arm_scmi: Add support for clock_enable_latency Cristian Marussi 2022-02-01 17:16 ` Cristian Marussi 2022-02-01 17:16 ` [PATCH v2 9/9] [RFC] clk: scmi: Support atomic clock enable/disable API Cristian Marussi 2022-02-01 17:16 ` 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=20220201171601.53316-4-cristian.marussi@arm.com \ --to=cristian.marussi@arm.com \ --cc=Jonathan.Cameron@Huawei.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=peter.hilber@opensynergy.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.