Re: [RFC PATCH] virtio_balloon: add param to skip adjusting pages

2021-11-17 Thread Michael S. Tsirkin
On Thu, Nov 18, 2021 at 10:25:45AM +0900, David Stevens wrote:
> On Wed, Nov 17, 2021 at 10:32 PM Michael S. Tsirkin  wrote:
> >
> > On Wed, Nov 17, 2021 at 07:06:34PM +0900, David Stevens wrote:
> > > From: David Stevens 
> > >
> > > Add a module parameters to virtio_balloon to allow specifying whether or
> > > not the driver should call adjust_managed_page_count. If the parameter
> > > is set, it overrides the default behavior inferred from the deflate on
> > > OOM flag. This allows the balloon to operate without changing the amount
> > > of memory visible to userspace via /proc/meminfo or sysinfo, even on a
> > > system that cannot set the default on OOM flag.
> > >
> > > The motivation for this patch is to allow userspace to more accurately
> > > take advantage of virtio_balloon's cooperative memory control on a
> > > system without the ability to use deflate on OOM. As it stands,
> > > userspace has no way to know how much memory may be available on such a
> > > system, which makes tasks such as sizing caches impossible.
> > >
> > > When deflate on OOM is not enabled, the current behavior of the
> > > virtio_balloon more or less resembles hotplugging individual pages, at
> > > least from an accounting perspective. This is basically hardcoding the
> > > requirement that totalram_pages must be available to the guest
> > > immediately, regardless of what the host does. While that is a valid
> > > policy, on Linux (which supports memory overcommit) with virtio_balloon
> > > (which is designed to facilitate overcommit in the host), it is not the
> > > only possible policy.
> > >
> > > The param added by this patch allows the guest to operate under the
> > > assumption that pages in the virtio_balloon will generally be made
> > > available when needed. This assumption may not always hold, but when it
> > > is violated, the guest will just fall back to the normal mechanisms for
> > > dealing with overcommitted memory.
> > >
> > > Signed-off-by: David Stevens 
> > > ---
> > >
> > > Another option to achieve similar behavior would be to add a new feature
> > > flag that would be used in conjunction with DEFLATE_ON_OOM to determine
> > > whether or not adjust_managed_page_count should be called. However, I
> > > feel that this sort of policy decision is a little outside the scope of
> > > what the virtio_balloon API deals with.
> > >
> > > ---
> > >  drivers/virtio/virtio_balloon.c | 23 ++-
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c 
> > > b/drivers/virtio/virtio_balloon.c
> > > index c22ff0117b46..17dac286899c 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -133,6 +133,21 @@ static const struct virtio_device_id id_table[] = {
> > >   { 0 },
> > >  };
> > >
> > > +static char *adjust_pages = "";
> > > +module_param(adjust_pages, charp, 0);
> > > +MODULE_PARM_DESC(adjust_pages, "Whether or not pages in the balloon 
> > > should be removed from the managed page count");
> > > +
> > > +static bool should_adjust_pages(struct virtio_balloon *vb)
> > > +{
> > > + if (!strcmp(adjust_pages, "always"))
> > > + return true;
> > > + else if (!strcmp(adjust_pages, "never"))
> > > + return false;
> > > +
> > > + return !virtio_has_feature(vb->vdev,
> > > +VIRTIO_BALLOON_F_DEFLATE_ON_OOM);
> > > +}
> > > +
> > >  static u32 page_to_balloon_pfn(struct page *page)
> > >  {
> > >   unsigned long pfn = page_to_pfn(page);
> > > @@ -243,8 +258,7 @@ static unsigned fill_balloon(struct virtio_balloon 
> > > *vb, size_t num)
> > >
> > >   set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> > >   vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > - if (!virtio_has_feature(vb->vdev,
> > > - VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + if (should_adjust_pages(vb))
> > >   adjust_managed_page_count(page, -1);
> > >   vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
> > >   }
> > > @@ -264,8 +278,7 @@ static void release_pages_balloon(struct 
> > > virtio_balloon *vb,
> > >   struct page *page, *next;
> > >
> > >   list_for_each_entry_safe(page, next, pages, lru) {
> > > - if (!virtio_has_feature(vb->vdev,
> > > - VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + if (should_adjust_pages(vb))
> > >   adjust_managed_page_count(page, 1);
> > >   list_del(&page->lru);
> > >   put_page(page); /* balloon reference */
> > > @@ -775,7 +788,7 @@ static int virtballoon_migratepage(struct 
> > > balloon_dev_info *vb_dev_info,
> > > * managed page count when inflating, we have to fixup the count of
> > > * both involved zones.
> > > */
> > > - if (!virtio_has_feature(vb->vdev, VIRTIO_BAL

Re: [PATCH 01/29] nvdimm/pmem: move dax_attribute_group from dax to pmem

2021-11-17 Thread Dan Williams
On Tue, Nov 9, 2021 at 12:33 AM Christoph Hellwig  wrote:
>
> dax_attribute_group is only used by the pmem driver, and can avoid the
> completely pointless lookup by the disk name if moved there.  This
> leaves just a single caller of dax_get_by_host, so move dax_get_by_host
> into the same ifdef block as that caller.
>
> Signed-off-by: Christoph Hellwig 
> Reviewed-by: Dan Williams 
> Link: https://lore.kernel.org/r/20210922173431.2454024-3-...@lst.de
> Signed-off-by: Dan Williams 

This one already made v5.16-rc1.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH 03/29] dax: remove CONFIG_DAX_DRIVER

2021-11-17 Thread Dan Williams
On Tue, Nov 9, 2021 at 12:33 AM Christoph Hellwig  wrote:
>
> CONFIG_DAX_DRIVER only selects CONFIG_DAX now, so remove it.

Applied.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH 02/29] dm: make the DAX support dependend on CONFIG_FS_DAX

2021-11-17 Thread Dan Williams
On Tue, Nov 9, 2021 at 12:33 AM Christoph Hellwig  wrote:
>
> The device mapper DAX support is all hanging off a block device and thus
> can't be used with device dax.  Make it depend on CONFIG_FS_DAX instead
> of CONFIG_DAX_DRIVER.  This also means that bdev_dax_pgoff only needs to
> be built under CONFIG_FS_DAX now.
>

Applied, fixed the spelling of 'dependent' in the subject and picked
up Mike's Ack from the previous send:

https://lore.kernel.org/r/yyasbvuorceds...@redhat.com

Christoph, any particular reason you did not pick up the tags from the
last posting?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [RFC PATCH] virtio_balloon: add param to skip adjusting pages

2021-11-17 Thread Michael S. Tsirkin
On Wed, Nov 17, 2021 at 07:06:34PM +0900, David Stevens wrote:
> From: David Stevens 
> 
> Add a module parameters to virtio_balloon to allow specifying whether or
> not the driver should call adjust_managed_page_count. If the parameter
> is set, it overrides the default behavior inferred from the deflate on
> OOM flag. This allows the balloon to operate without changing the amount
> of memory visible to userspace via /proc/meminfo or sysinfo, even on a
> system that cannot set the default on OOM flag.
> 
> The motivation for this patch is to allow userspace to more accurately
> take advantage of virtio_balloon's cooperative memory control on a
> system without the ability to use deflate on OOM. As it stands,
> userspace has no way to know how much memory may be available on such a
> system, which makes tasks such as sizing caches impossible.
> 
> When deflate on OOM is not enabled, the current behavior of the
> virtio_balloon more or less resembles hotplugging individual pages, at
> least from an accounting perspective. This is basically hardcoding the
> requirement that totalram_pages must be available to the guest
> immediately, regardless of what the host does. While that is a valid
> policy, on Linux (which supports memory overcommit) with virtio_balloon
> (which is designed to facilitate overcommit in the host), it is not the
> only possible policy.
> 
> The param added by this patch allows the guest to operate under the
> assumption that pages in the virtio_balloon will generally be made
> available when needed. This assumption may not always hold, but when it
> is violated, the guest will just fall back to the normal mechanisms for
> dealing with overcommitted memory.
> 
> Signed-off-by: David Stevens 
> ---
> 
> Another option to achieve similar behavior would be to add a new feature
> flag that would be used in conjunction with DEFLATE_ON_OOM to determine
> whether or not adjust_managed_page_count should be called. However, I
> feel that this sort of policy decision is a little outside the scope of
> what the virtio_balloon API deals with.
> 
> ---
>  drivers/virtio/virtio_balloon.c | 23 ++-
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index c22ff0117b46..17dac286899c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -133,6 +133,21 @@ static const struct virtio_device_id id_table[] = {
>   { 0 },
>  };
>  
> +static char *adjust_pages = "";
> +module_param(adjust_pages, charp, 0);
> +MODULE_PARM_DESC(adjust_pages, "Whether or not pages in the balloon should 
> be removed from the managed page count");
> +
> +static bool should_adjust_pages(struct virtio_balloon *vb)
> +{
> + if (!strcmp(adjust_pages, "always"))
> + return true;
> + else if (!strcmp(adjust_pages, "never"))
> + return false;
> +
> + return !virtio_has_feature(vb->vdev,
> +VIRTIO_BALLOON_F_DEFLATE_ON_OOM);
> +}
> +
>  static u32 page_to_balloon_pfn(struct page *page)
>  {
>   unsigned long pfn = page_to_pfn(page);
> @@ -243,8 +258,7 @@ static unsigned fill_balloon(struct virtio_balloon *vb, 
> size_t num)
>  
>   set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
>   vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> - if (!virtio_has_feature(vb->vdev,
> - VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> + if (should_adjust_pages(vb))
>   adjust_managed_page_count(page, -1);
>   vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE;
>   }
> @@ -264,8 +278,7 @@ static void release_pages_balloon(struct virtio_balloon 
> *vb,
>   struct page *page, *next;
>  
>   list_for_each_entry_safe(page, next, pages, lru) {
> - if (!virtio_has_feature(vb->vdev,
> - VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> + if (should_adjust_pages(vb))
>   adjust_managed_page_count(page, 1);
>   list_del(&page->lru);
>   put_page(page); /* balloon reference */
> @@ -775,7 +788,7 @@ static int virtballoon_migratepage(struct 
> balloon_dev_info *vb_dev_info,
> * managed page count when inflating, we have to fixup the count of
> * both involved zones.
> */
> - if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM) &&
> + if (should_adjust_pages(vb) &&
>   page_zone(page) != page_zone(newpage)) {
>   adjust_managed_page_count(page, 1);
>   adjust_managed_page_count(newpage, -1);

A problem here is that the host also cares: IIUC
with VIRTIO_BALLOON_F_STATS_VQ we are sending the info
about page counts to host, changing what the stats
mean.

So I suspect we need a device feature for this at least
to control this aspect.



> -- 
> 2.34.0.rc2.393.gf8c9666880-goog

_

Re: [PATCH] virtio-blk: modify the value type of num in virtio_queue_rq()

2021-11-17 Thread Stefano Garzarella

On Wed, Nov 17, 2021 at 06:39:55AM +, cgel@gmail.com wrote:

From: Ye Guojin 

This was found by coccicheck:
./drivers/block/virtio_blk.c, 334, 14-17, WARNING Unsigned expression
compared with zero  num < 0



We should add the Fixes tag:

Fixes: 02746e26c39e ("virtio-blk: avoid preallocating big SGL for data")


Reported-by: Zeal Robot 
Signed-off-by: Ye Guojin 
---
drivers/block/virtio_blk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 97bf051a50ce..eed1666eff31 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -316,7 +316,7 @@ static blk_status_t virtio_queue_rq(struct 
blk_mq_hw_ctx *hctx,

struct request *req = bd->rq;
struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
unsigned long flags;
-   unsigned int num;
+   int num;
int qid = hctx->queue_num;
bool notify = false;
blk_status_t status;
--
2.25.1



The patch LGTM.

With the Fixes tag added:

Reviewed-by: Stefano Garzarella 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization