Re: [PATCH] Cache->Cursor may point to Slab

["Durrant, Paul" <xadimgnik@xxxxxxxxx>]

On 20/05/2024 09:21, Durrant, Paul wrote:
> On 03/05/2024 11:33, Owen Smith wrote:
> > Its possible that the Cursor refers to the only Slab that has no
> > allocated objects, but there are other slabs which are fully
> > allocated. The current ASSERTions are not valid, and needs rework.
> >
> > Also adds more comments and ASSERTions to validate whats going on.
> >
> > Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> > ---
> >   src/xenbus/cache.c | 16 +++++++++++-----
> >   1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/xenbus/cache.c b/src/xenbus/cache.c
> > index 6229aed..821f66a 100644
> > --- a/src/xenbus/cache.c
> > +++ b/src/xenbus/cache.c
> > @@ -479,20 +479,25 @@ CacheDestroySlab(
> >       //
> >       // The only reason the cursor should be pointing at this slab is
> > -    // if it is the only one in the list.
> > +    // if it is the only one in the list with no allocated objects.
> Now that we don't spill empty slabs immediately, I think the cursor 
> could actually point at any one of several empty slabs.
> >       //
> >       if (Cache->Cursor == &Slab->ListEntry) {
> >           ASSERT3P(Slab->ListEntry.Flink, ==, &Cache->SlabList);
> > -        ASSERT3P(Slab->ListEntry.Blink, ==, &Cache->SlabList);
> > +        // Slab->ListEntry.Blink could refer to a fully allocated Slab
> > +        // Set Cursor to SlabList, so next CacheGet allocates a new Slab
> >           Cache->Cursor = &Cache->SlabList;
> >       }
> >       RemoveEntryList(&Slab->ListEntry);
> > +    // If Cache->Cursor == &Cache->SlabList,
> > +    //     either; all entries in SlabList are fully allocated; or
> > +    //             SlabList is empty
> > +    // If Cache->Cursor != &Cache->SlabList,
> > +    //     Cache->Cursor is the first Slab that is not fully allocated
> > -    ASSERT(IMPLY(Cache->Cursor == &Cache->SlabList,
> > -                 IsListEmpty(&Cache->SlabList)));
> > +    CacheAudit(Cache);
Actually, I think this might cause a problem. The cursor is reset to the 
anchor above but the audit could find a slab in the list that is not 
fully occupied, I think.
> > -    Index = CacheMaskSize(Slab->Allocated);
> > +    Index = CacheMaskSize(Slab->Constructed);
> >       while (--Index >= 0) {
> >           PVOID Object = (PVOID)&Slab->Buffer[Index * Cache->Size];
> > @@ -501,6 +506,7 @@ CacheDestroySlab(
> >               __CacheMaskClear(Slab->Constructed, Index);
> >           }
> >       }
> > +    ASSERT3U(CacheMaskCount(Slab->Constructed), ==, 0);
> >       ASSERT(Cache->CurrentSlabs != 0);
> >       InterlockedDecrement(&Cache->CurrentSlabs);
> Acked-by: Paul Durrant <paul@xxxxxxx>
I'll withdraw that and take a closer look at this.

  Paul
>