Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Duyck <alexander.duyck@gmail.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: David Hildenbrand <david@redhat.com>,
	kvm list <kvm@vger.kernel.org>,
	 LKML <linux-kernel@vger.kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	 Michal Hocko <mhocko@kernel.org>, linux-mm <linux-mm@kvack.org>,
	 Andrew Morton <akpm@linux-foundation.org>,
	Mel Gorman <mgorman@techsingularity.net>,
	 Vlastimil Babka <vbabka@suse.cz>,
	Yang Zhang <yang.zhang.wz@gmail.com>,
	 Nitesh Narayan Lal <nitesh@redhat.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	 Pankaj Gupta <pagupta@redhat.com>,
	Rik van Riel <riel@surriel.com>,
	lcapitulino@redhat.com,  Dave Hansen <dave.hansen@intel.com>,
	"Wang, Wei W" <wei.w.wang@intel.com>,
	 Andrea Arcangeli <aarcange@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	 Dan Williams <dan.j.williams@intel.com>,
	 Alexander Duyck <alexander.h.duyck@linux.intel.com>,
	Oscar Salvador <osalvador@suse.de>
Subject: Re: [PATCH v14 6/6] virtio-balloon: Add support for providing unused page reports to host
Date: Sun, 1 Dec 2019 10:25:09 -0800
Message-ID: <CAKgT0UeCZJqEjyngQxtdFcaEP1Ei_X6ydNyE_4Jfr6cs=zKkiw@mail.gmail.com> (raw)
In-Reply-To: <20191201041731-mutt-send-email-mst@kernel.org>

On Sun, Dec 1, 2019 at 3:46 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 29, 2019 at 01:13:32PM -0800, Alexander Duyck wrote:
> > On Thu, Nov 28, 2019 at 7:26 AM David Hildenbrand <david@redhat.com> wrote:
> > >
> > > On 19.11.19 22:46, Alexander Duyck wrote:
> > > > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > > >
> > > > Add support for the page reporting feature provided by virtio-balloon.
> > > > Reporting differs from the regular balloon functionality in that is is
> > > > much less durable than a standard memory balloon. Instead of creating a
> > > > list of pages that cannot be accessed the pages are only inaccessible
> > > > while they are being indicated to the virtio interface. Once the
> > > > interface has acknowledged them they are placed back into their respective
> > > > free lists and are once again accessible by the guest system.
> > >
> > > Maybe add something like "In contrast to ordinary balloon
> > > inflation/deflation, the guest can reuse all reported pages immediately
> > > after reporting has finished, without having to notify the hypervisor
> > > about it (e.g., VIRTIO_BALLOON_F_MUST_TELL_HOST does not apply)."
> >
> > Okay. I'll make a note of it for next version.
>
>
> VIRTIO_BALLOON_F_MUST_TELL_HOST is IMHO misdocumented.
> It states:
>         VIRTIO_BALLOON_F_MUST_TELL_HOST (0) Host has to be told before pages from the balloon are
>         used.
> but really balloon always told host. The difference is in timing,
> historically balloon gave up pages before sending the
> message and before waiting for the buffer to be used by host.
>
> I think this feature can be the same if we want.

Okay. I'll still probably try to document the behavior a bit better though.

> > > [...]
> > >
> > > >  /*
> > > >   * Balloon device works in 4K page units.  So each page is pointed to by
> > > > @@ -37,6 +38,9 @@
> > > >  #define VIRTIO_BALLOON_FREE_PAGE_SIZE \
> > > >       (1 << (VIRTIO_BALLOON_FREE_PAGE_ORDER + PAGE_SHIFT))
> > > >
> > > > +/*  limit on the number of pages that can be on the reporting vq */
> > > > +#define VIRTIO_BALLOON_VRING_HINTS_MAX       16
> > >
> > > Maybe rename that from HINTS to REPORTS
> >
> > I'll fix it for the next version.
> >
> > > > +
> > > >  #ifdef CONFIG_BALLOON_COMPACTION
> > > >  static struct vfsmount *balloon_mnt;
> > > >  #endif
> > > > @@ -46,6 +50,7 @@ enum virtio_balloon_vq {
> > > >       VIRTIO_BALLOON_VQ_DEFLATE,
> > > >       VIRTIO_BALLOON_VQ_STATS,
> > > >       VIRTIO_BALLOON_VQ_FREE_PAGE,
> > > > +     VIRTIO_BALLOON_VQ_REPORTING,
> > > >       VIRTIO_BALLOON_VQ_MAX
> > > >  };
> > > >
> > > > @@ -113,6 +118,10 @@ struct virtio_balloon {
> > > >
> > > >       /* To register a shrinker to shrink memory upon memory pressure */
> > > >       struct shrinker shrinker;
> > > > +
> > > > +     /* Unused page reporting device */
> > >
> > > Sounds like the device is unused :D
> > >
> > > "Device info for reporting unused pages" ?
> > >
> > > I am in general wondering, should we rename "unused" to "free". I.e.,
> > > "free page reporting" instead of "unused page reporting"? Or what was
> > > the motivation behind using "unused" ?
> >
> > I honestly don't remember why I chose "unused" at this point. I can
> > switch over to "free" if that is what is preferred.
> >
> > Looking over the code a bit more I suspect the reason for avoiding it
> > is because free page hinting also mentioned reporting in a few spots.
> >
> > > > +     struct virtqueue *reporting_vq;
> > > > +     struct page_reporting_dev_info pr_dev_info;
> > > >  };
> > > >
> > > >  static struct virtio_device_id id_table[] = {
> > > > @@ -152,6 +161,32 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq)
> > > >
> > > >  }
> > > >
> > > > +void virtballoon_unused_page_report(struct page_reporting_dev_info *pr_dev_info,
> > > > +                                 unsigned int nents)
> > > > +{
> > > > +     struct virtio_balloon *vb =
> > > > +             container_of(pr_dev_info, struct virtio_balloon, pr_dev_info);
> > > > +     struct virtqueue *vq = vb->reporting_vq;
> > > > +     unsigned int unused, err;
> > > > +
> > > > +     /* We should always be able to add these buffers to an empty queue. */
> > >
> > > This comment somewhat contradicts the error handling (and comment)
> > > below. Maybe just drop it?
> > >
> > > > +     err = virtqueue_add_inbuf(vq, pr_dev_info->sg, nents, vb,
> > > > +                               GFP_NOWAIT | __GFP_NOWARN);
> > > > +
> > > > +     /*
> > > > +      * In the extremely unlikely case that something has changed and we
> > > > +      * are able to trigger an error we will simply display a warning
> > > > +      * and exit without actually processing the pages.
> > > > +      */
> > > > +     if (WARN_ON(err))
> > > > +             return;
> > >
> > > Maybe WARN_ON_ONCE? (to not flood the log on recurring errors)
> >
> > Actually I might need to tweak things here a bit. It occurs to me that
> > this can fail for more than just there not being space in the ring. I
> > forgot that DMA mapping needs to also occur so in the case of a DMA
> > mapping failure we would also see an error.
>
> Balloon assumes DMA mapping is bypassed right now:
>
> static int virtballoon_validate(struct virtio_device *vdev)
> {
>         if (!page_poisoning_enabled())
>                 __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
>
>         __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
>
> ^^^^^^^^
>
>
>         return 0;
> }
>
> I don't think it can work with things like a bounce buffer.

Right. It wouldn't work with a bounce buffer. I was thinking more of
something like an IOMMU. So it sounds like the device is doing direct
map always anyway.

In any case I will add some logic so that if we encounter an error we
will just abort the reporting. That way if another user has some issue
like that it can be dealt with sooner and we can avoid flagging pages
as reported that are not.

- Alex


  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19 21:46 [PATCH v14 0/6] mm / virtio: Provide support for unused page reporting Alexander Duyck
2019-11-19 21:46 ` [PATCH v14 1/6] mm: Adjust shuffle code to allow for future coalescing Alexander Duyck
2019-11-27 13:52   ` Mel Gorman
2019-11-19 21:46 ` [PATCH v14 2/6] mm: Use zone and order instead of free area in free_list manipulators Alexander Duyck
2019-11-27 13:54   ` Mel Gorman
2019-11-19 21:46 ` [PATCH v14 3/6] mm: Introduce Reported pages Alexander Duyck
2019-11-27 15:24   ` Mel Gorman
2019-11-27 17:22     ` Alexander Duyck
2019-11-27 18:35       ` Mel Gorman
2019-11-27 21:55         ` Alexander Duyck
2019-11-28  9:22           ` Mel Gorman
2019-11-29 19:25             ` Alexander Duyck
2019-11-19 21:46 ` [PATCH v14 4/6] mm: Add unused page reporting documentation Alexander Duyck
2019-11-19 21:46 ` [PATCH v14 5/6] virtio-balloon: Pull page poisoning config out of free page hinting Alexander Duyck
2019-11-19 21:46 ` [PATCH v14 6/6] virtio-balloon: Add support for providing unused page reports to host Alexander Duyck
2019-11-28 15:25   ` David Hildenbrand
2019-11-28 17:00     ` Michael S. Tsirkin
2019-12-04 17:48       ` Alexander Duyck
2019-12-04 17:53       ` Alexander Duyck
2019-11-29 21:13     ` Alexander Duyck
2019-12-01 11:46       ` Michael S. Tsirkin
2019-12-01 18:25         ` Alexander Duyck [this message]
2019-12-02 10:43       ` David Hildenbrand
2019-11-19 21:54 ` [PATCH v14 QEMU 1/3] virtio-ballon: Implement support for page poison tracking feature Alexander Duyck
2019-11-19 21:54 ` [PATCH v14 QEMU 2/3] virtio-balloon: Add bit to notify guest of unused page reporting Alexander Duyck
2019-11-19 21:54 ` [PATCH v14 QEMU 3/3] virtio-balloon: Provide a interface for " Alexander Duyck
2019-11-26 12:20 ` [PATCH v14 0/6] mm / virtio: Provide support " David Hildenbrand
2019-11-26 16:45   ` Alexander Duyck
2019-11-27 10:01     ` David Hildenbrand
2019-11-27 17:36       ` Alexander Duyck
2019-11-27 17:37         ` David Hildenbrand

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKgT0UeCZJqEjyngQxtdFcaEP1Ei_X6ydNyE_4Jfr6cs=zKkiw@mail.gmail.com' \
    --to=alexander.duyck@gmail.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@intel.com \
    --cc=david@redhat.com \
    --cc=konrad.wilk@oracle.com \
    --cc=kvm@vger.kernel.org \
    --cc=lcapitulino@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mgorman@techsingularity.net \
    --cc=mhocko@kernel.org \
    --cc=mst@redhat.com \
    --cc=nitesh@redhat.com \
    --cc=osalvador@suse.de \
    --cc=pagupta@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=riel@surriel.com \
    --cc=vbabka@suse.cz \
    --cc=wei.w.wang@intel.com \
    --cc=willy@infradead.org \
    --cc=yang.zhang.wz@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git