Message-Id: <f08d5585-0a26-4c98-be1a-c0fd89ccd916@app.fastmail.com>
Date: Wed, 06 Aug 2025 17:50:42 +0200
From: "Pierre Barre" <pierre@...re.sh>
To: asmadeus <asmadeus@...ewreck.org>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>
Cc: "Christian Schoenebeck" <linux_oss@...debyte.com>, v9fs@...ts.linux.dev,
 ericvh@...nel.org, lucho@...kov.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] 9p: Use kvmalloc for message buffers

Hi Everyone,

If I submit a patch based on what has been discussed above, is it likely to be accepted? Unfortunately, in my current setup, I am encountering this issue quite frequently, and users of my servers are having a hard time making sense of the error.

Thank you.

Best,
Pierre

On Thu, Jul 31, 2025, at 02:36, Pierre Barre wrote:
>> First - thanks for starting this thread, large contiguous allocations
>> has been a problem reported regularly in the past, and no-one has had
>> the energy to address it yet, but it's definitely something worth
>> tackling.
>
> Glad that I didn't just create noise. I implemented 9P in addition to 
> NFS in my server because fsyncing on NFS didn't give me what I wanted 
> (fsync doesn't call commit when write is communicated to be "durable" 
> by the server, but that's a story for an other day...). 
>
> I am very grateful that this client implementation exists.
>
> On Thu, Jul 31, 2025, at 00:07, asmadeus@...ewreck.org wrote:
>> (Added Willy in To, if you have time to advise on what's appropriate
>> wrt. memory allocation here to use either as a contiguous virtual memory
>> buffer or a scatterlist for various kind of dma used by transports
>> that'd be appreciated)
>>
>>
>> First - thanks for starting this thread, large contiguous allocations
>> has been a problem reported regularly in the past, and no-one has had
>> the energy to address it yet, but it's definitely something worth
>> tackling.
>>
>>
>> Pierre Barre wrote on Wed, Jul 30, 2025 at 10:16:13PM +0200:
>>> As a middle-ground, would it be acceptable to add a transport
>>> capability flag indicating whether the transport requires contiguous
>>> memory for DMA?
>>
>> I think such a flag will be needed even if we do what Christian suggested:
>>>> So a more reasonable solution instead might be using kvmalloc(), as suggested
>>>> by you, and adjusting the individual transports such that they translate a
>>>> virtual memory address to a list of physical addresses via e.g.
>>>> vmalloc_to_page() if needed.
>>
>> Start with a requires contiguous flag set for all transports except
>> trans_fd (I'm not sure about usb, but I'm pretty sure all the rest need
>> it); then each transport can figure out how to get a scatterlist or
>> something they need from the allocation, so we don't need to have a big
>> bang that breaks everything at the same time.
>>
>> I'm sure virtio's implem will come soon enough, but I don't see anyone
>> working on RDMA or xen so fast.
>>
>>> 1. Add a P9_TRANS_REQUIRES_CONTIGUOUS flag to struct p9_trans_module
>>> 2. Set this flag for virtio, xen, and rdma transports
>>
>> This is a nit but I'd rather the flag go the other way around, e.g. no
>> flag means requires contiguous and it's only set after confirming the
>> transport works
>> (I just looked at usb and struct usb_request makes me think some drivers
>> use dma, so, yeah..)
>>
>> We can always turn it around later if the majority of transports handle
>> it.
>>
>>
>>> 3. Modify p9_fcall_init to check the flag:
>>>      if (c->trans_mod->caps & P9_TRANS_REQUIRES_CONTIGUOUS)
>>>         fc->sdata = kmalloc(alloc_msize, GFP_NOFS);
>>>      else
>>>         fc->sdata = kvmalloc(alloc_msize, GFP_NOFS);
>>
>> I'm also curious if there's something more appropriate than kvmalloc wrt
>> using either as a contiguous virtual memory buffer or a scatterlist, but
>> I think this is fine for now unless someone knows of something more
>> appropriate.
>>
>>
>> Thanks,
>> -- 
>> Dominique Martinet | Asmadeus

Powered by blists - more mailing lists