Message-ID: <YZl+eD6r0iIGzS43@codewreck.org>
Date:   Sun, 21 Nov 2021 08:02:16 +0900
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Christian Schoenebeck <linux_oss@...debyte.com>,
        Nikolay Kichukov <nikolay@...um.net>
Cc:     v9fs-developer@...ts.sourceforge.net, netdev@...r.kernel.org,
        Eric Van Hensbergen <ericvh@...il.com>,
        Latchesar Ionkov <lucho@...kov.net>,
        Greg Kurz <groug@...d.org>, Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH v3 6/7] 9p/trans_virtio: support larger msize values


Christian,

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 :)



Nikolay,

> > > (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.

-- 
Dominique

Powered by blists - more mailing lists