On Wed, Jul 21, 2021 at 09:14:31PM +0100, Alex Bennée wrote:
>
> Mathieu Poirier <mathieu.poir...@linaro.org> writes:
>
> > This patch provides the vhost-user backend implementation to work
> > in tandem with the vhost-user-rng implementation of the QEMU VMM.
> >
> > It uses the vhost-user API so that other VMM can re-use the interface
> > without having to write the driver again.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poir...@linaro.org>
>
> Try the following patch which creates a nested main loop and runs it
> until the g_timeout_add fires again.
>
> --8<---------------cut here---------------start------------->8---
> tools/virtio/vhost-user-rng: avoid mutex by using nested main loop
>
> As we are blocking anyway all we really need to do is run a main loop
> until the timer fires and the data is consumed.
>
Right, I made the implemenation blocking to be as close as possible to what
virtio-rng does.
I took a look at your patch below and it should do the trick. Testing yielded
the same results as my solution so this is good. To me the nested loop is a
little unorthodox to solve this kind of problem but it has less lines of code
and avoids spinning a new thread to deal with the timer.
I'll send another revision.
Thanks for the review,
Mathieu
> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>
> 1 file changed, 30 insertions(+), 76 deletions(-)
> tools/vhost-user-rng/main.c | 106 +++++++++++++-------------------------------
>
> modified tools/vhost-user-rng/main.c
> @@ -42,13 +42,10 @@
>
> typedef struct {
> VugDev dev;
> - struct itimerspec ts;
> - timer_t rate_limit_timer;
> - pthread_mutex_t rng_mutex;
> - pthread_cond_t rng_cond;
> int64_t quota_remaining;
> - bool activate_timer;
> + guint timer;
> GMainLoop *loop;
> + GMainLoop *blocked;
> } VuRNG;
>
> static gboolean print_cap, verbose;
> @@ -59,66 +56,26 @@ static gint source_fd, socket_fd = -1;
> static uint32_t period_ms = 1 << 16;
> static uint64_t max_bytes = INT64_MAX;
>
> -static void check_rate_limit(union sigval sv)
> +static gboolean check_rate_limit(gpointer user_data)
> {
> - VuRNG *rng = sv.sival_ptr;
> - bool wakeup = false;
> + VuRNG *rng = (VuRNG *) user_data;
>
> - pthread_mutex_lock(&rng->rng_mutex);
> - /*
> - * The timer has expired and the guest has used all available
> - * entropy, which means function vu_rng_handle_request() is waiting
> - * on us. As such wake it up once we're done here.
> - */
> - if (rng->quota_remaining == 0) {
> - wakeup = true;
> + if (rng->blocked) {
> + g_info("%s: called while blocked", __func__);
> + g_main_loop_quit(rng->blocked);
> }
> -
> /*
> * Reset the entropy available to the guest and tell function
> * vu_rng_handle_requests() to start the timer before using it.
> */
> rng->quota_remaining = max_bytes;
> - rng->activate_timer = true;
> - pthread_mutex_unlock(&rng->rng_mutex);
> -
> - if (wakeup) {
> - pthread_cond_signal(&rng->rng_cond);
> - }
> -}
> -
> -static void setup_timer(VuRNG *rng)
> -{
> - struct sigevent sev;
> - int ret;
> -
> - memset(&rng->ts, 0, sizeof(struct itimerspec));
> - rng->ts.it_value.tv_sec = period_ms / 1000;
> - rng->ts.it_value.tv_nsec = (period_ms % 1000) * 1000000;
> -
> - /*
> - * Call function check_rate_limit() as if it was the start of
> - * a new thread when the timer expires.
> - */
> - sev.sigev_notify = SIGEV_THREAD;
> - sev.sigev_notify_function = check_rate_limit;
> - sev.sigev_value.sival_ptr = rng;
> - /* Needs to be NULL if defaults attributes are to be used. */
> - sev.sigev_notify_attributes = NULL;
> - ret = timer_create(CLOCK_MONOTONIC, &sev, &rng->rate_limit_timer);
> - if (ret < 0) {
> - fprintf(stderr, "timer_create() failed\n");
> - }
> -
> + return true;
> }
>
> -
> /* Virtio helpers */
> static uint64_t rng_get_features(VuDev *dev)
> {
> - if (verbose) {
> - g_info("%s: replying", __func__);
> - }
> + g_info("%s: replying", __func__);
> return 0;
> }
>
> @@ -137,7 +94,7 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
> VuVirtq *vq = vu_get_queue(dev, qidx);
> VuVirtqElement *elem;
> size_t to_read;
> - int len, ret;
> + int len;
>
> for (;;) {
> /* Get element in the vhost virtqueue */
> @@ -149,24 +106,21 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
> /* Get the amount of entropy to read from the vhost server */
> to_read = elem->in_sg[0].iov_len;
>
> - pthread_mutex_lock(&rng->rng_mutex);
> -
> /*
> * We have consumed all entropy available for this time slice.
> * Wait for the timer (check_rate_limit()) to tell us about the
> * start of a new time slice.
> */
> if (rng->quota_remaining == 0) {
> - pthread_cond_wait(&rng->rng_cond, &rng->rng_mutex);
> - }
> -
> - /* Start the timer if the last time slice has expired */
> - if (rng->activate_timer == true) {
> - rng->activate_timer = false;
> - ret = timer_settime(rng->rate_limit_timer, 0, &rng->ts, NULL);
> - if (ret < 0) {
> - fprintf(stderr, "timer_settime() failed\n");
> - }
> + g_assert(!rng->blocked);
> + rng->blocked =
> g_main_loop_new(g_main_loop_get_context(rng->loop), false);
> + g_info("attempting to consume %ld bytes but no quota left (%s)",
> + to_read,
> + g_main_loop_is_running(rng->loop) ? "running" : "not
> running");
> + g_main_loop_run(rng->blocked);
> + g_info("return from blocked loop: %ld", rng->quota_remaining);
> + g_main_loop_unref(rng->blocked);
> + rng->blocked = false;
> }
>
> /* Make sure we don't read more than it's available */
> @@ -183,8 +137,6 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
>
> rng->quota_remaining -= len;
>
> - pthread_mutex_unlock(&rng->rng_mutex);
> -
> vu_queue_push(dev, vq, elem, len);
> free(elem);
> }
> @@ -373,6 +325,7 @@ int main(int argc, char *argv[])
> * can add it's GSource watches.
> */
> rng.loop = g_main_loop_new(NULL, FALSE);
> + rng.blocked = NULL;
>
> if (!vug_init(&rng.dev, 1, g_socket_get_fd(socket),
> panic, &vuiface)) {
> @@ -380,24 +333,25 @@ int main(int argc, char *argv[])
> exit(EXIT_FAILURE);
> }
>
> - rng.quota_remaining = max_bytes;
> - rng.activate_timer = true;
> - pthread_mutex_init(&rng.rng_mutex, NULL);
> - pthread_cond_init(&rng.rng_cond, NULL);
> - setup_timer(&rng);
> -
> if (verbose) {
> - g_info("period_ms: %d tv_sec: %ld tv_nsec: %lu\n",
> - period_ms, rng.ts.it_value.tv_sec, rng.ts.it_value.tv_nsec);
> + g_log_set_handler(NULL, G_LOG_LEVEL_MASK, g_log_default_handler,
> NULL);
> + g_setenv("G_MESSAGES_DEBUG", "all", true);
> + } else {
> + g_log_set_handler(NULL,
> + G_LOG_LEVEL_WARNING | G_LOG_LEVEL_CRITICAL |
> G_LOG_LEVEL_ERROR,
> + g_log_default_handler, NULL);
> }
>
> + rng.quota_remaining = max_bytes;
> + rng.timer = g_timeout_add(period_ms, check_rate_limit, &rng);
> + g_info("period_ms: %"PRId32", timer %d\n", period_ms, rng.timer);
> +
> g_message("entering main loop, awaiting messages");
> g_main_loop_run(rng.loop);
> g_message("finished main loop, cleaning up");
>
> g_main_loop_unref(rng.loop);
> vug_deinit(&rng.dev);
> - timer_delete(rng.rate_limit_timer);
> close(source_fd);
> unlink(socket_path);
> }
> --8<---------------cut here---------------end--------------->8---
>
> --
> Alex Bennée
