Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Christian Schoenebeck <linux_oss@crudebyte.com>
To:     Dominique Martinet <asmadeus@codewreck.org>,
        Kent Overstreet <kent.overstreet@gmail.com>
Cc:     linux-kernel@vger.kernel.org, v9fs-developer@lists.sourceforge.net,
        Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>
Subject: Re: [PATCH 3/3] 9p: Add mempools for RPCs
Date:   Sat, 09 Jul 2022 16:21:46 +0200
Message-ID: <1690934.P4sCSNuWZQ@silver>
In-Reply-To: <Yskxs4uQ4v8l7Zb9@codewreck.org>
References: <20220704010945.C230AC341C7@smtp.kernel.org> <72042449.h6Bkk5LDil@silver>
 <Yskxs4uQ4v8l7Zb9@codewreck.org>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Samstag, 9. Juli 2022 09:43:47 CEST Dominique Martinet wrote:
> I've taken the mempool patches to 9p-next
>=20
> Christian Schoenebeck wrote on Mon, Jul 04, 2022 at 03:56:55PM +0200:
> >> (I appreciate the need for testing, but this feels much less risky than
> >> the iovec series we've had recently... Famous last words?)
> >=20
> > Got it, consider my famous last words dropped. ;-)
>=20
> Ok, so I think you won this one...
>=20
> Well -- when testing normally it obviously works well, performance wise
> is roughly the same (obviously since it tries to allocate from slab
> first and in normal case that will work)
>=20
> When I tried gaming it with very low memory though I thought it worked
> well, but I managed to get a bunch of processes stuck in mempool_alloc
> with no obvious tid waiting for a reply.
> I had the bright idea of using fio with io_uring and interestingly the
> uring worker doesn't show up in ps or /proc/<pid>, but with qemu's gdb
> and lx-ps I could find a bunch of iou-wrk-<pid> that are all with
> similar stacks
>    1   =E2=94=82 [<0>] mempool_alloc+0x136/0x180
>    2   =E2=94=82 [<0>] p9_fcall_init+0x63/0x80 [9pnet]
>    3   =E2=94=82 [<0>] p9_client_prepare_req+0xa9/0x290 [9pnet]
>    4   =E2=94=82 [<0>] p9_client_rpc+0x64/0x610 [9pnet]
>    5   =E2=94=82 [<0>] p9_client_write+0xcb/0x210 [9pnet]
>    6   =E2=94=82 [<0>] v9fs_file_write_iter+0x4d/0xc0 [9p]
>    7   =E2=94=82 [<0>] io_write+0x129/0x2c0
>    8   =E2=94=82 [<0>] io_issue_sqe+0xa1/0x25b0
>    9   =E2=94=82 [<0>] io_wq_submit_work+0x90/0x190
>   10   =E2=94=82 [<0>] io_worker_handle_work+0x211/0x550
>   11   =E2=94=82 [<0>] io_wqe_worker+0x2c5/0x340
>   12   =E2=94=82 [<0>] ret_from_fork+0x1f/0x30
>=20
> or, and that's the interesting part
>    1   =E2=94=82 [<0>] mempool_alloc+0x136/0x180
>    2   =E2=94=82 [<0>] p9_fcall_init+0x63/0x80 [9pnet]
>    3   =E2=94=82 [<0>] p9_client_prepare_req+0xa9/0x290 [9pnet]
>    4   =E2=94=82 [<0>] p9_client_rpc+0x64/0x610 [9pnet]
>    5   =E2=94=82 [<0>] p9_client_flush+0x81/0xc0 [9pnet]
>    6   =E2=94=82 [<0>] p9_client_rpc+0x591/0x610 [9pnet]
>    7   =E2=94=82 [<0>] p9_client_write+0xcb/0x210 [9pnet]
>    8   =E2=94=82 [<0>] v9fs_file_write_iter+0x4d/0xc0 [9p]
>    9   =E2=94=82 [<0>] io_write+0x129/0x2c0
>   10   =E2=94=82 [<0>] io_issue_sqe+0xa1/0x25b0
>   11   =E2=94=82 [<0>] io_wq_submit_work+0x90/0x190
>   12   =E2=94=82 [<0>] io_worker_handle_work+0x211/0x550
>   13   =E2=94=82 [<0>] io_wqe_worker+0x2c5/0x340
>   14   =E2=94=82 [<0>] ret_from_fork+0x1f/0x30
>=20
> The problem is these flushes : the same task is holding a buffer for the
> original rpc and tries to get a new one, but waits for someone to free
> and.. obviously there isn't anyone (I cound 11 flushes pending, so more
> than the minimum number of buffers we'd expect from the mempool, and I
> don't think we missed any free)
>=20
> Now I'm not sure what's best here.
> The best thing to do would probably to just tell the client it can't use
> the mempools for flushes -- the flushes are rare and will use small
> buffers with your smaller allocations patch; I bet I wouldn't be able to
> reproduce that anymore but it should probably just forbid the mempool
> just in case.

So the problem is that one task ends up with more than 1 request at a time,=
=20
and the buffer is allocated and associated per request, not per task. If I =
am=20
not missing something, then this scenario (>1 request simultaniously per ta=
sk)=20
currently may actually only happen with p9_client_flush() calls. Which=20
simplifies the problem.

So probably the best way would be to simply flip the call order such that=20
p9_tag_remove() is called before p9_client_flush(), similar to how it's=20
already done with p9_client_clunk() calls?

> Anyway, I'm not comfortable with this patch right now, a hang is worse
> than an allocation failure warning.

As you already mentioned, with the pending 'net/9p: allocate appropriate=20
reduced message buffers' patch those hangs should not happen, as Tflush wou=
ld=20
then just kmalloc() a small buffer. But I would probably still fix this iss=
ue=20
here nevertheless, as it might hurt in other ways in future. Shouldn't be t=
oo=20
much noise to swap the call order, right?

> > > > How about I address the already discussed issues and post a v5 of
> > > > those
> > > > patches this week and then we can continue from there?
> > >=20
> > > I would have been happy to rebase your patches 9..12 on top of Kent's
> > > this weekend but if you want to refresh them this week we can continue
> > > from there, sure.
> >=20
> > I'll rebase them on master and address what we discussed so far. Then
> > we'll
> > see.
>=20
> FWIW and regarding the other thread with virito queue sizes, I was only
> considering the later patches with small RPCs for this merge window.

I would also recommend to leave out the virtio patches, yes.

> Shall we try to focus on that first, and then revisit the virtio and
> mempool patches once that's done?

Your call. I think both ways are viable.

Best regards,
Christian Schoenebeck