archive mirror
 help / color / mirror / Atom feed
From: Dominique Martinet <>
To: Christian Schoenebeck <>,
	Nikolay Kichukov <>
	Eric Van Hensbergen <>,
	Latchesar Ionkov <>, Greg Kurz <>,
	Vivek Goyal <>
Subject: Re: [PATCH v3 6/7] 9p/trans_virtio: support larger msize values
Date: Sun, 21 Nov 2021 08:02:16 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> <2717208.9V0g2NVZY4@silver>


Christian Schoenebeck wrote on Sat, Nov 20, 2021 at 03:46:19PM +0100:
> > So in practice, you will be capped at 2MB as that is the biggest the
> > slab will be able to hand over in a single chunk.
> I did not encounter a 2MB limit here. But kmalloc() clearly has a 4MB limit, 
> so when trying an msize larger than 4MB it inevitably causes a memory 
> allocation error. In my tests this allocation error would always happen 
> immediately at mount time causing an instant kernel oops.

Interesting, I was assuming it'd have the same limit.
There must be some fallback path I didn't know about... I wonder if it
handles non-contiguous memory ranges too then, in which case it's not as
bad as I'd have expected depending on how finely it's willing to sew
things back together: I'll check

> > Ideally we'd only allocate on an as-need basis, most of the protocol
> > calls bound how much data is supposed to come back and we know how much
> > we want to send (it's a format string actually, but we can majorate it
> > quite easily), so one would need to adjust all protocol calls to pass
> > this info to p9_client_rpc/p9_client_zc_rpc so it only allocates buffers
> > as required, if necessary in multiple reasonably-sized segments (I'd
> > love 2MB hugepages-backed folios...), and have all transports use these
> > buffers.
> It is not that bad in sense of pending work. One major thing that needs to be 
> done is to cap the majority of 9p message types to allocate only as much as 
> they need, which is for most message types <8k. Right now they always simply 
> kmalloc(msize), which hurts with increasing msize values. That task does not 
> need many changes though.

Yes, that could be a first step.
Although frankly as I said if we're going to do this, we actual can
majorate the actual max for all operations pretty easily thanks to the
count parameter -- I guess it's a bit more work but we can put arbitrary
values (e.g. 8k for all the small stuff) instead of trying to figure it
out more precisely; I'd just like the code path to be able to do it so
we only do that rechurn once.

Note I've been rather aggressive with checkpatch warning fixes in my
last update for 5.16, hopefully it won't conflict too much with your
work... Let me deal with conflicts if it's a problem.

> > I have a rough idea on how to do all this but honestly less than 0 time
> > for that, so happy to give advices or review any patch, but it's going
> > to be a lot of work that stand in the way of really big IOs.
> Reviews of the patches would actually help a lot to bring this overall effort 
> forward, but probably rather starting with the upcoming next version of the 
> kernel patches, not this old v3.

Happy to review anything you send over, yes :)


> > > (Not sure about this, I'll test these patches tomorrow, but since
> > > something failed I'm not surprised you have less than what you could
> > > have here: what do you get with a more reasonable value like 1M
> > > first?)
> It worked with 1MB, I can stick to this for the time being.
> Are the kernel patches supposed to be included in the KVM host kernel or
> would the guest kernel suffice?

The patches are only required in the guest.


  parent reply	other threads:[~2021-11-20 23:17 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 16:00 [PATCH v3 0/7] net/9p: remove msize limit in virtio transport Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 1/7] net/9p: show error message if user 'msize' cannot be satisfied Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 2/7] 9p/trans_virtio: separate allocation of scatter gather list Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 3/7] 9p/trans_virtio: turn amount of sg lists into runtime info Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 4/7] 9p/trans_virtio: introduce struct virtqueue_sg Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 5/7] net/9p: add trans_maxsize to struct p9_client Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 6/7] 9p/trans_virtio: support larger msize values Christian Schoenebeck
2021-11-20 11:20   ` Nikolay Kichukov
2021-11-20 11:45     ` Dominique Martinet
2021-11-20 14:46       ` Christian Schoenebeck
2021-11-20 21:28         ` Nikolay Kichukov
2021-11-20 23:02         ` Dominique Martinet [this message]
2021-11-21 16:57           ` Christian Schoenebeck
2021-11-21 22:12             ` Dominique Martinet
2021-11-22 13:32               ` Christian Schoenebeck
2021-11-22 22:35                 ` Dominique Martinet
2021-11-24 12:22                   ` Christian Schoenebeck
2021-09-22 16:00 ` [PATCH v3 7/7] 9p/trans_virtio: resize sg lists to whatever is possible Christian Schoenebeck

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:

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

  git send-email \ \ \ \ \ \ \ \ \ \ \

* 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).