Re: [PATCH V2 2/7] xen/grants: support allocating consecutive grants

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.05.22 19:00, Boris Ostrovsky wrote:


Hello Boris


> On 5/16/22 1:59 AM, Juergen Gross wrote:
> > On 14.05.22 04:34, Boris Ostrovsky wrote:
> > > On 5/13/22 1:33 AM, Juergen Gross wrote:
> > > > On 12.05.22 22:01, Boris Ostrovsky wrote:
> > > > > On 5/7/22 2:19 PM, Oleksandr Tyshchenko wrote:
> > > > > > +/* 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.
> > >
> > > > > > +            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?
> >
> > > 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@xxxxxxxxxx>
Thanks!



> -boris
--
Regards,

Oleksandr Tyshchenko