From: Stefano Stabellini <sstabellini@kernel.org> To: Dominique Martinet <asmadeus@codewreck.org> Cc: Stefano Stabellini <sstabellini@kernel.org>, jgross@suse.com, lucho@ionkov.net, ericvh@gmail.com, rminnich@sandia.gov, linux-kernel@vger.kernel.org, v9fs-developer@lists.sourceforge.net, xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com, Stefano Stabellini <stefano.stabellini@xilinx.com> Subject: Re: [V9fs-developer] [PATCH] 9p/xen: increase XEN_9PFS_RING_ORDER Date: Wed, 20 May 2020 13:47:46 -0700 (PDT) [thread overview] Message-ID: <alpine.DEB.2.21.2005201340310.27502@sstabellini-ThinkPad-T480s> (raw) In-Reply-To: <20200520193647.GA17565@nautica> On Wed, 20 May 2020, Dominique Martinet wrote: > Stefano Stabellini wrote on Wed, May 20, 2020: > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the > > max allowed by the protocol. > > > > We can't assume that all backends will support order 9. The xenstore > > property max-ring-page-order specifies the max order supported by the > > backend. We'll use max-ring-page-order for the size of the ring. > > > > This means that the size of the ring is not static > > (XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an > > argument and base the calculation on the order chosen at setup time. > > > > > > Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the > > original value. We need to divide it by 2 because we have two rings > > coming off the same order allocation: the in and out rings. This was a > > mistake in the original code. Also divide it further by 2 because we > > don't want a single request/reply to fill up the entire ring. There can > > be multiple requests/replies outstanding at any given time and if we use > > the full ring with one, we risk forcing the backend to wait for the > > client to read back more replies before continuing, which is not > > performant. > > Sounds good to me overall. A couple of comments inline. > Also worth noting I need to rebuild myself a test setup so might take a > bit of time to actually run tests, but I might just trust you on this > one for now if it builds with no new warning... Looks like it would > probably work :p > > > [...] > > @@ -264,7 +265,7 @@ static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r) > > > > static struct p9_trans_module p9_xen_trans = { > > .name = "xen", > > - .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT), > > + .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT - 2), > > .def = 1, > > .create = p9_xen_create, > > .close = p9_xen_close, > > [...] > > @@ -401,8 +405,10 @@ static int xen_9pfs_front_probe(struct xenbus_device *dev, > > return -EINVAL; > > max_ring_order = xenbus_read_unsigned(dev->otherend, > > "max-ring-page-order", 0); > > - if (max_ring_order < XEN_9PFS_RING_ORDER) > > - return -EINVAL; > > + if (max_ring_order > XEN_9PFS_RING_ORDER) > > + max_ring_order = XEN_9PFS_RING_ORDER; > > (If there are backends with very small max_ring_orders, we no longer > error out when we encounter one, it might make sense to add a min > define? Although to be honest 9p works with pretty small maxsizes so I > don't see much reason to error out, and even order 0 will be one page > worth.. I hope there is no xenbus that small though :)) Your point is valid but the size calculation (XEN_FLEX_RING_SIZE) should work correctly even with order 0: (1UL << ((0) + XEN_PAGE_SHIFT - 1)) = 1 << (12 - 1) = 2048 So I am thinking that the protocol should still work correctly, although the performance might be undesirable. FYI The smallest backend I know of has order 6. > > + if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order)) > > + p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order); > > So base maxsize initial value is 1 << (order + page_shift - 2) ; but > this is 1 << (order + page_shift - 1) -- I agree with the logic you gave > in commit message so would think this needs to be shifted down one more > like the base value as well. > What do you think? Yes, you are right, thanks for noticing this! I meant to do that here too but somehow forgot. This should be: p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2;
WARNING: multiple messages have this Message-ID (diff)
From: Stefano Stabellini <sstabellini@kernel.org> To: Dominique Martinet <asmadeus@codewreck.org> Cc: jgross@suse.com, lucho@ionkov.net, Stefano Stabellini <sstabellini@kernel.org>, xen-devel@lists.xenproject.org, ericvh@gmail.com, linux-kernel@vger.kernel.org, v9fs-developer@lists.sourceforge.net, rminnich@sandia.gov, boris.ostrovsky@oracle.com, Stefano Stabellini <stefano.stabellini@xilinx.com> Subject: Re: [V9fs-developer] [PATCH] 9p/xen: increase XEN_9PFS_RING_ORDER Date: Wed, 20 May 2020 13:47:46 -0700 (PDT) [thread overview] Message-ID: <alpine.DEB.2.21.2005201340310.27502@sstabellini-ThinkPad-T480s> (raw) In-Reply-To: <20200520193647.GA17565@nautica> On Wed, 20 May 2020, Dominique Martinet wrote: > Stefano Stabellini wrote on Wed, May 20, 2020: > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the > > max allowed by the protocol. > > > > We can't assume that all backends will support order 9. The xenstore > > property max-ring-page-order specifies the max order supported by the > > backend. We'll use max-ring-page-order for the size of the ring. > > > > This means that the size of the ring is not static > > (XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an > > argument and base the calculation on the order chosen at setup time. > > > > > > Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the > > original value. We need to divide it by 2 because we have two rings > > coming off the same order allocation: the in and out rings. This was a > > mistake in the original code. Also divide it further by 2 because we > > don't want a single request/reply to fill up the entire ring. There can > > be multiple requests/replies outstanding at any given time and if we use > > the full ring with one, we risk forcing the backend to wait for the > > client to read back more replies before continuing, which is not > > performant. > > Sounds good to me overall. A couple of comments inline. > Also worth noting I need to rebuild myself a test setup so might take a > bit of time to actually run tests, but I might just trust you on this > one for now if it builds with no new warning... Looks like it would > probably work :p > > > [...] > > @@ -264,7 +265,7 @@ static irqreturn_t xen_9pfs_front_event_handler(int irq, void *r) > > > > static struct p9_trans_module p9_xen_trans = { > > .name = "xen", > > - .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT), > > + .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT - 2), > > .def = 1, > > .create = p9_xen_create, > > .close = p9_xen_close, > > [...] > > @@ -401,8 +405,10 @@ static int xen_9pfs_front_probe(struct xenbus_device *dev, > > return -EINVAL; > > max_ring_order = xenbus_read_unsigned(dev->otherend, > > "max-ring-page-order", 0); > > - if (max_ring_order < XEN_9PFS_RING_ORDER) > > - return -EINVAL; > > + if (max_ring_order > XEN_9PFS_RING_ORDER) > > + max_ring_order = XEN_9PFS_RING_ORDER; > > (If there are backends with very small max_ring_orders, we no longer > error out when we encounter one, it might make sense to add a min > define? Although to be honest 9p works with pretty small maxsizes so I > don't see much reason to error out, and even order 0 will be one page > worth.. I hope there is no xenbus that small though :)) Your point is valid but the size calculation (XEN_FLEX_RING_SIZE) should work correctly even with order 0: (1UL << ((0) + XEN_PAGE_SHIFT - 1)) = 1 << (12 - 1) = 2048 So I am thinking that the protocol should still work correctly, although the performance might be undesirable. FYI The smallest backend I know of has order 6. > > + if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order)) > > + p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order); > > So base maxsize initial value is 1 << (order + page_shift - 2) ; but > this is 1 << (order + page_shift - 1) -- I agree with the logic you gave > in commit message so would think this needs to be shifted down one more > like the base value as well. > What do you think? Yes, you are right, thanks for noticing this! I meant to do that here too but somehow forgot. This should be: p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2;
next prev parent reply other threads:[~2020-05-20 20:47 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-20 18:41 [PATCH] 9p/xen: increase XEN_9PFS_RING_ORDER Stefano Stabellini 2020-05-20 18:41 ` Stefano Stabellini 2020-05-20 19:36 ` [V9fs-developer] " Dominique Martinet 2020-05-20 19:36 ` Dominique Martinet 2020-05-20 20:47 ` Stefano Stabellini [this message] 2020-05-20 20:47 ` Stefano Stabellini
Reply instructions: You may reply publicly 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=alpine.DEB.2.21.2005201340310.27502@sstabellini-ThinkPad-T480s \ --to=sstabellini@kernel.org \ --cc=asmadeus@codewreck.org \ --cc=boris.ostrovsky@oracle.com \ --cc=ericvh@gmail.com \ --cc=jgross@suse.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lucho@ionkov.net \ --cc=rminnich@sandia.gov \ --cc=stefano.stabellini@xilinx.com \ --cc=v9fs-developer@lists.sourceforge.net \ --cc=xen-devel@lists.xenproject.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.