Re: [PATCH v3 3/4] vhost-user-rng: backend: Add RNG vhost-user daemon implementation

[Alex Bennée]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On Wed, Jul 21, 2021 at 09:14:31PM +0100, Alex Bennée wrote:
>> 
>> Mathieu Poirier <mathieu.poirier@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.poirier@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.

It might be worth considering just running a g_main_context_iteration()
of the main loop:

  
https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#g-main-context-iteration

and then you could avoid the hassle of having a special blocking loop
(and having to special case the quit). However you can't be sure the
quota has filled up so you need to test on the loop. My hack up (on top
of this patch):

--8<---------------cut here---------------start------------->8---
modified   tools/vhost-user-rng/main.c
@@ -45,7 +45,6 @@ typedef struct {
     int64_t quota_remaining;
     guint timer;
     GMainLoop *loop;
-    GMainLoop *blocked;
 } VuRNG;
 
 static gboolean print_cap, verbose;
@@ -59,10 +58,8 @@ static uint64_t max_bytes = INT64_MAX;
 static gboolean check_rate_limit(gpointer user_data)
 {
     VuRNG *rng = (VuRNG *) user_data;
-
-    if (rng->blocked) {
-        g_info("%s: called while blocked", __func__);
-        g_main_loop_quit(rng->blocked);
+    if (!rng->quota_remaining) {
+        g_info("%s: replenishing empty quota", __func__);
     }
     /*
      * Reset the entropy available to the guest and tell function
@@ -112,15 +109,12 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
          * start of a new time slice.
          */
         if (rng->quota_remaining == 0) {
-            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;
+            g_info("blocking on consuming %ld bytes as no quota", to_read);
+            do {
+                g_main_context_iteration(g_main_loop_get_context(rng->loop),
+                                         true);
+                g_info("return from blocked loop: %ld", rng->quota_remaining);
+            } while (!rng->quota_remaining);
         }
 
         /* Make sure we don't read more than it's available */
@@ -325,7 +319,6 @@ 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)) {
--8<---------------cut here---------------end--------------->8---

And then with:

  10:24 root@buster/aarch64  [~] >dd if=/dev/hwrng of=/dev/null status=progress
  77312 bytes (77 kB, 76 KiB) copied, 150 s, 0.5 kB/s

and:

  ./tools/vhost-user-rng/vhost-user-rng --socket-path vrng.sock -v -m 512 -p 
1000

I was seeing:

  vhost-user-rng-INFO: 10:27:14.453: blocking on consuming 64 bytes as no quota
  vhost-user-rng-INFO: 10:27:14.453: return from blocked loop: 0
  vhost-user-rng-INFO: 10:27:15.451: check_rate_limit: replenishing empty quota
  vhost-user-rng-INFO: 10:27:15.451: return from blocked loop: 512
  vhost-user-rng-INFO: 10:27:15.457: blocking on consuming 64 bytes as no quota
  vhost-user-rng-INFO: 10:27:15.457: return from blocked loop: 0
  vhost-user-rng-INFO: 10:27:16.453: check_rate_limit: replenishing empty quota
  vhost-user-rng-INFO: 10:27:16.453: return from blocked loop: 512
  vhost-user-rng-INFO: 10:27:16.456: blocking on consuming 64 bytes as no quota
  vhost-user-rng-INFO: 10:27:16.456: return from blocked loop: 0
  vhost-user-rng-INFO: 10:27:17.454: check_rate_limit: replenishing empty quota
  vhost-user-rng-INFO: 10:27:17.454: return from blocked loop: 512
  vhost-user-rng-INFO: 10:27:17.456: blocking on consuming 64 bytes as no quota
  vhost-user-rng-INFO: 10:27:17.456: return from blocked loop: 0

which seemed reasonable enough...

>
> I'll send another revision.
>
> Thanks for the review,
> Mathieu
>
>> Signed-off-by: Alex Bennée <alex.bennee@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


-- 
Alex Bennée