Re: git: f1c5a2e3a625 - main - virtio_random: Pipeline fetching the data

On 9/26/23 7:08 PM, Andrew Turner wrote:
> Is there a plan to MFC this to 14? Without it I’m seeing 50% CPU on a dual core arm64 Hetzner VM.
> 
> Andrew

I can merge it sure, I've just been AFK most of last week and am still
catching up a bit.

>> On 5 Sep 2023, at 17:00, John Baldwin <jhb@freebsd.org> wrote:
>>
>> The branch main has been updated by jhb:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=f1c5a2e3a625053e2b70d5b1777d849a4d9328f2
>>
>> commit f1c5a2e3a625053e2b70d5b1777d849a4d9328f2
>> Author:     John-Mark Gurney <jmg@FreeBSD.org>
>> AuthorDate: 2023-09-05 15:59:43 +0000
>> Commit:     John Baldwin <jhb@FreeBSD.org>
>> CommitDate: 2023-09-05 15:59:43 +0000
>>
>>     virtio_random: Pipeline fetching the data
>>
>>     Queue an initial fetch of data during attach and after every read
>>     rather than synchronously fetching data and polling for completion.
>>
>>     If data has not been returned from an previous fetch during read,
>>     just return EAGAIN rather than blocking.
>>
>>     Co-authored-by: John Baldwin <jhb@FreeBSD.org>
>>
>>     Reviewed by:    markj
>>     Differential Revision:  https://reviews.freebsd.org/D41656
>> ---
>> sys/dev/virtio/random/virtio_random.c | 73 +++++++++++++++++++++--------------
>> 1 file changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/sys/dev/virtio/random/virtio_random.c b/sys/dev/virtio/random/virtio_random.c
>> index c02b5c98cece..d54e2e6b70d4 100644
>> --- a/sys/dev/virtio/random/virtio_random.c
>> +++ b/sys/dev/virtio/random/virtio_random.c
>> @@ -54,6 +54,8 @@ struct vtrnd_softc {
>> 	struct virtqueue	*vtrnd_vq;
>> 	eventhandler_tag	 eh;
>> 	bool			 inactive;
>> +	struct sglist		 *vtrnd_sg;
>> +	uint32_t		 *vtrnd_value;
>> };
>>
>> static int	vtrnd_modevent(module_t, int, void *);
>> @@ -67,6 +69,7 @@ static int	vtrnd_negotiate_features(struct vtrnd_softc *);
>> static int	vtrnd_setup_features(struct vtrnd_softc *);
>> static int	vtrnd_alloc_virtqueue(struct vtrnd_softc *);
>> static int	vtrnd_harvest(struct vtrnd_softc *, void *, size_t *);
>> +static void	vtrnd_enqueue(struct vtrnd_softc *sc);
>> static unsigned	vtrnd_read(void *, unsigned);
>>
>> #define VTRND_FEATURES	0
>> @@ -138,12 +141,17 @@ static int
>> vtrnd_attach(device_t dev)
>> {
>> 	struct vtrnd_softc *sc, *exp;
>> +	size_t len;
>> 	int error;
>>
>> 	sc = device_get_softc(dev);
>> 	sc->vtrnd_dev = dev;
>> 	virtio_set_feature_desc(dev, vtrnd_feature_desc);
>>
>> +	len = sizeof(*sc->vtrnd_value) * HARVESTSIZE;
>> +	sc->vtrnd_value = malloc_aligned(len, len, M_DEVBUF, M_WAITOK);
>> +	sc->vtrnd_sg = sglist_build(sc->vtrnd_value, len, M_WAITOK);
>> +
>> 	error = vtrnd_setup_features(sc);
>> 	if (error) {
>> 		device_printf(dev, "cannot setup features\n");
>> @@ -174,6 +182,8 @@ vtrnd_attach(device_t dev)
>> 	sc->inactive = false;
>> 	random_source_register(&random_vtrnd);
>>
>> +	vtrnd_enqueue(sc);
>> +
>> fail:
>> 	if (error)
>> 		vtrnd_detach(dev);
>> @@ -185,6 +195,7 @@ static int
>> vtrnd_detach(device_t dev)
>> {
>> 	struct vtrnd_softc *sc;
>> +	uint32_t rdlen;
>>
>> 	sc = device_get_softc(dev);
>> 	KASSERT(
>> @@ -197,7 +208,13 @@ vtrnd_detach(device_t dev)
>> 		sc->eh = NULL;
>> 	}
>> 	random_source_deregister(&random_vtrnd);
>> +
>> +	/* clear the queue */
>> +	virtqueue_poll(sc->vtrnd_vq, &rdlen);
>> +
>> 	atomic_store_explicit(&g_vtrnd_softc, NULL, memory_order_release);
>> +	sglist_free(sc->vtrnd_sg);
>> +	zfree(sc->vtrnd_value, M_DEVBUF);
>> 	return (0);
>> }
>>
>> @@ -251,49 +268,45 @@ vtrnd_alloc_virtqueue(struct vtrnd_softc *sc)
>> 	return (virtio_alloc_virtqueues(dev, 0, 1, &vq_info));
>> }
>>
>> +static void
>> +vtrnd_enqueue(struct vtrnd_softc *sc)
>> +{
>> +	struct virtqueue *vq;
>> +	int error __diagused;
>> +
>> +	vq = sc->vtrnd_vq;
>> +
>> +	KASSERT(virtqueue_empty(vq), ("%s: non-empty queue", __func__));
>> +
>> +	error = virtqueue_enqueue(vq, sc, sc->vtrnd_sg, 0, 1);
>> +	KASSERT(error == 0, ("%s: virtqueue_enqueue returned error: %d",
>> +	    __func__, error));
>> +
>> +	virtqueue_notify(vq);
>> +}
>> +
>> static int
>> vtrnd_harvest(struct vtrnd_softc *sc, void *buf, size_t *sz)
>> {
>> -	struct sglist_seg segs[1];
>> -	struct sglist sg;
>> 	struct virtqueue *vq;
>> -	uint32_t value[HARVESTSIZE] __aligned(sizeof(uint32_t) * HARVESTSIZE);
>> +	void *cookie;
>> 	uint32_t rdlen;
>> -	int error;
>> -
>> -	_Static_assert(sizeof(value) < PAGE_SIZE, "sglist assumption");
>>
>> 	if (sc->inactive)
>> 		return (EDEADLK);
>>
>> -	sglist_init(&sg, 1, segs);
>> -	error = sglist_append(&sg, value, *sz);
>> -	if (error != 0)
>> -		panic("%s: sglist_append error=%d", __func__, error);
>> -
>> 	vq = sc->vtrnd_vq;
>> -	KASSERT(virtqueue_empty(vq), ("%s: non-empty queue", __func__));
>> -
>> -	error = virtqueue_enqueue(vq, buf, &sg, 0, 1);
>> -	if (error != 0)
>> -		return (error);
>> -
>> -	/*
>> -	 * Poll for the response, but the command is likely already
>> -	 * done when we return from the notify.
>> -	 */
>> -	virtqueue_notify(vq);
>> -	virtqueue_poll(vq, &rdlen);
>>
>> -	if (rdlen > *sz)
>> -		panic("%s: random device wrote %zu bytes beyond end of provided"
>> -		    " buffer %p:%zu", __func__, (size_t)rdlen - *sz,
>> -		    (void *)value, *sz);
>> -	else if (rdlen == 0)
>> +	cookie = virtqueue_dequeue(vq, &rdlen);
>> +	if (cookie == NULL)
>> 		return (EAGAIN);
>> +	KASSERT(cookie == sc, ("%s: cookie mismatch", __func__));
>> +
>> 	*sz = MIN(rdlen, *sz);
>> -	memcpy(buf, value, *sz);
>> -	explicit_bzero(value, *sz);
>> +	memcpy(buf, sc->vtrnd_value, *sz);
>> +
>> +	vtrnd_enqueue(sc);
>> +
>> 	return (0);
>> }
>>
>>
> 

-- 
John Baldwin