On 16.05.22 19:00, Boris Ostrovsky wrote:


Hello Boris

On 5/16/22 1:59 AM, Juergen Gross wrote:

quoted

On 14.05.22 04:34, Boris Ostrovsky wrote:

quoted

On 5/13/22 1:33 AM, Juergen Gross wrote:

quoted

On 12.05.22 22:01, Boris Ostrovsky wrote:

quoted

On 5/7/22 2:19 PM, Oleksandr Tyshchenko wrote:

quoted

quoted

quoted

+/* Rebuilds the free grant list and tries to find count 
consecutive entries. */
+static int get_free_seq(unsigned int count)
+{
+    int ret = -ENOSPC;
+    unsigned int from, to;
+    grant_ref_t *last;
+
+    gnttab_free_tail_ptr = &gnttab_free_head;
+    last = &gnttab_free_head;
+
+    for (from = find_first_bit(gnttab_free_bitmap, gnttab_size);
+         from < gnttab_size;
+         from = find_next_bit(gnttab_free_bitmap, gnttab_size, 
to + 1)) {
+        to = find_next_zero_bit(gnttab_free_bitmap, gnttab_size,
+                    from + 1);
+        if (ret < 0 && to - from >= count) {
+            ret = from;
+            bitmap_clear(gnttab_free_bitmap, ret, count);
+            from += count;
+            gnttab_free_count -= count;

IIUIC we can have multiple passes over this, meaning that the 
gnttab_free_count may be decremented more than once. Is that 
intentional?

After the first pass decrementing gnttab_free_cnt, ret will no
longer be less than zero, so this can be hit only once.

Oh, yes, of course.

quoted

quoted

quoted

+            if (from == to)
+                continue;
+        }
+
+        while (from < to) {
+            *last = from;
+            last = __gnttab_entry(from);
+            gnttab_last_free = from;
+            from++;
+        }

I have been looking at this loop and I can't understand what it is 
doing ;-( Can you enlighten me?

It is recreating the free list in order to have it properly sorted.
This is needed to make sure that the free tail has the maximum
possible size (you can take the tail off the list without having
to worry about breaking the linked list because of references into
the tail).

So let's say we have the (one-dimensional) table of length 13

idx    ..    2    3  ...  10  11  12

grant       12   11        2  -1   3


and gnttab_free_head is 10. I.e. the free list is 2, 12, 3, 11.

You meant 10, 2, 12, 3, 11, I guess?

quoted

What will this look like after the 2 iterations of the outer loop?

idx    ..    2    3  ...  10  11  12

grant        3   10       11  12  -1

with gnttab_free_head being 2, i.e the free list is now 2, 3, 10, 11, 
12.

OK, thanks, that helped. I couldn't link the free chunks in my head


With the error handling in gnttab_init() fixed

yes, this is a diff that I am going to apply for the next version:


[snip]

@@ -1596,19 +1601,20 @@ static int gnttab_expand(unsigned int req_entries)
  int gnttab_init(void)
  {
         int i;
-       unsigned long max_nr_grant_frames;
+       unsigned long max_nr_grant_frames, max_nr_grefs;
         unsigned int max_nr_glist_frames, nr_glist_frames;
         int ret;

         gnttab_request_version();
         max_nr_grant_frames = gnttab_max_grant_frames();
+       max_nr_grefs = max_nr_grant_frames *
+ gnttab_interface->grefs_per_grant_frame;
         nr_grant_frames = 1;

         /* Determine the maximum number of frames required for the
          * grant reference free list on the current hypervisor.
          */
-       max_nr_glist_frames = (max_nr_grant_frames *
- gnttab_interface->grefs_per_grant_frame / RPP);
+       max_nr_glist_frames = max_nr_grefs / RPP;

         gnttab_list = kmalloc_array(max_nr_glist_frames,
                                     sizeof(grant_ref_t *),

@@ -1625,8 +1631,7 @@ int gnttab_init(void)
                 }
         }

-       i = gnttab_interface->grefs_per_grant_frame * max_nr_grant_frames;
-       gnttab_free_bitmap = bitmap_zalloc(i, GFP_KERNEL);
+       gnttab_free_bitmap = bitmap_zalloc(max_nr_grefs, GFP_KERNEL);
         if (!gnttab_free_bitmap) {
                 ret = -ENOMEM;
                 goto ini_nomem;

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Thanks!

-boris

-- 
Regards,

Oleksandr Tyshchenko


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel