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;
prev parent reply other threads:[~2020-05-20 20:47 UTC|newest]
Thread overview: 3+ 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 19:36 ` [V9fs-developer] " Dominique Martinet
2020-05-20 20:47 ` Stefano Stabellini [this message]
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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).