Re: And another patch...

[Samuel Thibault]

Hello,

Just to be sure: this is not only because of your previous patch
libpager: Do not flush in-core pages on offer
?

Samuel

Sergey Bugaev, le jeu. 06 mai 2021 20:58:29 +0300, a ecrit:
> There's yet another bug: libpager just throws away clean precious
> pages (those previously offered with pager_offer_page (precious = 1))
> upon receiving them from the kernel, since it mistakes them for
> evicted pages it's being notified about. This is obviously very
> problematic.
> 
> I'm attaching a patch that attempts to fix the issue, but I'm less
> sure about it. What do you think?
> 
> Sergey
> 
> -- >8 --
> Subject: [PATCH] libpager: Do not throw away precious pages
> 
> The kernel invokes memory_object_data_return () with dirty = 0 in two
> cases: if notification on eviction was requested, or when returning
> precious pages. Properly distinguish between the two cases, and only
> throw the clean page away in the former case.
> ---
>  libpager/data-return.c | 69 ++++++++++++++++--------------------------
>  libpager/offer-page.c  |  2 ++
>  libpager/priv.h        |  1 +
>  3 files changed, 29 insertions(+), 43 deletions(-)
> 
> diff --git a/libpager/data-return.c b/libpager/data-return.c
> index c0f5aaf7..59e3e956 100644
> --- a/libpager/data-return.c
> +++ b/libpager/data-return.c
> @@ -38,12 +38,12 @@ _pager_do_write_request (struct pager *p,
>    short *pm_entries;
>    int npages, i;
>    char *notified;
> +  char *omit_data;
>    error_t *pagerrs;
>    struct lock_request *lr;
>    struct lock_list {struct lock_request *lr;
>      struct lock_list *next;} *lock_list, *ll;
>    int wakeup;
> -  int omitdata = 0;
> 
>    if (!p
>        || p->port.class != _pager_class)
> @@ -78,6 +78,9 @@ _pager_do_write_request (struct pager *p,
>    npages = length / __vm_page_size;
>    pagerrs = alloca (npages * sizeof (error_t));
> 
> +  omit_data = alloca (npages * (sizeof *omit_data));
> +  memset (omit_data, 0, npages * (sizeof *omit_data));
> +
>    notified = alloca (npages * (sizeof *notified));
>  #ifndef NDEBUG
>    memset (notified, -1, npages * (sizeof *notified));
> @@ -90,23 +93,6 @@ _pager_do_write_request (struct pager *p,
> 
>    pm_entries = &p->pagemap[offset / __vm_page_size];
> 
> -  if (! dirty)
> -    {
> -      munmap ((void *) data, length);
> -      if (!kcopy) {
> -        /* Prepare notified array.  */
> -        for (i = 0; i < npages; i++)
> -          notified[i] = (p->notify_on_evict
> -                         && ! (pm_entries[i] & PM_PAGEINWAIT));
> -
> -        goto notify;
> -      }
> -      else {
> -        _pager_allow_termination (p);
> -        goto release_out;
> -      }
> -    }
> -
>    /* Make sure there are no other in-progress writes for any of these
>       pages before we begin.  This imposes a little more serialization
>       than we really have to require (because *all* future writes on
> @@ -115,27 +101,24 @@ _pager_do_write_request (struct pager *p,
>    /* XXX: Is this still needed?  */
>   retry:
>    for (i = 0; i < npages; i++)
> -    if (pm_entries[i] & PM_PAGINGOUT)
> -      {
> - pm_entries[i] |= PM_WRITEWAIT;
> - pthread_cond_wait (&p->wakeup, &p->interlock);
> - goto retry;
> +    {
> +      if ((initializing && (pm_entries[i] & PM_INIT)) ||
> +          (!dirty && !(pm_entries[i] & PM_PRECIOUS)))
> +        {
> +          omit_data[i] = 1;
> +          continue;
> +        }
> +      if (pm_entries[i] & PM_PAGINGOUT)
> +        {
> +          pm_entries[i] |= PM_WRITEWAIT;
> +          pthread_cond_wait (&p->wakeup, &p->interlock);
> +          goto retry;
>        }
> +    }
> 
>    /* Mark these pages as being paged out.  */
> -  if (initializing)
> -    {
> -      assert_backtrace (npages <= 32);
> -      for (i = 0; i < npages; i++)
> - {
> -  if (pm_entries[i] & PM_INIT)
> -    omitdata |= 1 << i;
> -  else
> -    pm_entries[i] |= PM_PAGINGOUT | PM_INIT;
> - }
> -    }
> -  else
> -    for (i = 0; i < npages; i++)
> +  for (i = 0; i < npages; i++)
> +    if (!omit_data[i])
>        pm_entries[i] |= PM_PAGINGOUT | PM_INIT;
> 
>    /* If this write occurs while a lock is pending, record
> @@ -162,7 +145,7 @@ _pager_do_write_request (struct pager *p,
>       but until the pager library interface is changed, this will have to do. 
> */
> 
>    for (i = 0; i < npages; i++)
> -    if (!(omitdata & (1 << i)))
> +    if (!omit_data[i])
>        pagerrs[i] = pager_write_page (p->upi,
>       offset + (vm_page_size * i),
>       data + (vm_page_size * i));
> @@ -175,9 +158,11 @@ _pager_do_write_request (struct pager *p,
>    wakeup = 0;
>    for (i = 0; i < npages; i++)
>      {
> -      if (omitdata & (1 << i))
> +      if (omit_data[i])
>   {
> -  notified[i] = 0;
> +  notified[i] = (p->notify_on_evict
> +                         && !kcopy
> +                         && ! (pm_entries[i] & PM_PAGEINWAIT));
>    continue;
>   }
> 
> @@ -205,14 +190,13 @@ _pager_do_write_request (struct pager *p,
>   }
>        else
>   {
> -  munmap ((void *) (data + (vm_page_size * i)),
> -  vm_page_size);
>    notified[i] = (! kcopy && p->notify_on_evict);
>    if (! kcopy)
>      pm_entries[i] &= ~PM_INCORE;
>   }
> 
> -      pm_entries[i] &= ~(PM_PAGINGOUT | PM_PAGEINWAIT | PM_WRITEWAIT);
> +      pm_entries[i] &= ~(PM_PAGINGOUT | PM_PAGEINWAIT
> +                       | PM_WRITEWAIT | PM_PRECIOUS);
>      }
> 
>    for (ll = lock_list; ll; ll = ll->next)
> @@ -222,7 +206,6 @@ _pager_do_write_request (struct pager *p,
>    if (wakeup)
>      pthread_cond_broadcast (&p->wakeup);
> 
> - notify:
>    _pager_allow_termination (p);
>    pthread_mutex_unlock (&p->interlock);
> 
> diff --git a/libpager/offer-page.c b/libpager/offer-page.c
> index 26f88ca3..392e83b8 100644
> --- a/libpager/offer-page.c
> +++ b/libpager/offer-page.c
> @@ -38,6 +38,8 @@ pager_offer_page (struct pager *p,
> 
>    short *pm_entry = &p->pagemap[offset / vm_page_size];
>    *pm_entry |= PM_INCORE;
> +  if (precious)
> +    *pm_entry |= PM_PRECIOUS;
> 
>    err = memory_object_data_supply (p->memobjcntl, offset, buf, vm_page_size, 
> 0,
>                                     writelock ? VM_PROT_WRITE : VM_PROT_NONE,
> diff --git a/libpager/priv.h b/libpager/priv.h
> index d9d76965..a5e22f36 100644
> --- a/libpager/priv.h
> +++ b/libpager/priv.h
> @@ -108,6 +108,7 @@ extern int _pager_page_errors[];
> 
>  /* Pagemap format */
>  /* These are binary state bits */
> +#define PM_PRECIOUS   0x0400 /* return even if not dirty */
>  #define PM_WRITEWAIT  0x0200 /* queue wakeup once write is done */
>  #define PM_INIT       0x0100    /* data has been written */
>  #define PM_INCORE     0x0080 /* kernel might have a copy */
> -- 
> 2.31.1
> 

-- 
Samuel
Accroche-toi au terminal, j'enlève le shell...
 -+- nojhan -+-