Message-ID: <20240827100053.2f5c3403@mordecai.tesarici.cz>
Date: Tue, 27 Aug 2024 10:00:53 +0200
From: Petr Tesařík <petr@...arici.cz>
To: Michael Kelley <mhklinux@...look.com>
Cc: "kbusch@...nel.org" <kbusch@...nel.org>, "axboe@...nel.dk"
 <axboe@...nel.dk>, "sagi@...mberg.me" <sagi@...mberg.me>,
 "James.Bottomley@...senPartnership.com"
 <James.Bottomley@...senPartnership.com>, "martin.petersen@...cle.com"
 <martin.petersen@...cle.com>, "kys@...rosoft.com" <kys@...rosoft.com>,
 "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>, "wei.liu@...nel.org"
 <wei.liu@...nel.org>, "decui@...rosoft.com" <decui@...rosoft.com>,
 "robin.murphy@....com" <robin.murphy@....com>, "hch@....de" <hch@....de>,
 "m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
 "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
 "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
 "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>
Subject: Re: [RFC 0/7] Introduce swiotlb throttling

On Tue, 27 Aug 2024 00:26:36 +0000
Michael Kelley <mhklinux@...look.com> wrote:

> From: Petr Tesařík <petr@...arici.cz> Sent: Monday, August 26, 2024 12:28 PM
> > 
> > On Mon, 26 Aug 2024 16:24:53 +0000
> > Michael Kelley <mhklinux@...look.com> wrote:
> >   
> > > From: Petr Tesařík <petr@...arici.cz> Sent: Saturday, August 24, 2024 1:06 PM  
> > > >
> > > > On Fri, 23 Aug 2024 20:40:16 +0000
> > > > Michael Kelley <mhklinux@...look.com> wrote:
> > > >  
> > > > > From: Petr Tesařík <petr@...arici.cz> Sent: Thursday, August 22, 2024 11:45 PM
> > > > >[...]  
> > > > > > > Discussion
> > > > > > > ==========
> > > > > > > * Since swiotlb isn't visible to device drivers, I've specifically
> > > > > > > named the DMA attribute as MAY_BLOCK instead of MAY_THROTTLE or
> > > > > > > something swiotlb specific. While this patch set consumes MAY_BLOCK
> > > > > > > only on the DMA direct path to do throttling in the swiotlb code,
> > > > > > > there might be other uses in the future outside of CoCo VMs, or
> > > > > > > perhaps on the IOMMU path.  
> > > > > >
> > > > > > I once introduced a similar flag and called it MAY_SLEEP. I chose
> > > > > > MAY_SLEEP, because there is already a might_sleep() annotation, but I
> > > > > > don't have a strong opinion unless your semantics is supposed to be
> > > > > > different from might_sleep(). If it is, then I strongly prefer
> > > > > > MAY_BLOCK to prevent confusing the two.  
> > > > >
> > > > > My intent is that the semantics are the same as might_sleep(). I
> > > > > vacillated between MAY_SLEEP and MAY_BLOCK. The kernel seems
> > > > > to treat "sleep" and "block" as equivalent, because blk-mq has
> > > > > the BLK_MQ_F_BLOCKING flag, and SCSI has the
> > > > > queuecommand_may_block flag that is translated to
> > > > > BLK_MQ_F_BLOCKING. So I settled on MAY_BLOCK, but as you
> > > > > point out, that's inconsistent with might_sleep(). Either way will
> > > > > be inconsistent somewhere, and I don't have a preference.  
> > > >
> > > > Fair enough. Let's stay with MAY_BLOCK then, so you don't have to
> > > > change it everywhere.
> > > >  
> > > > >[...]  
> > > > > > > Open Topics
> > > > > > > ===========
> > > > > > > 1. swiotlb allocations from Xen and the IOMMU code don't make use
> > > > > > > of throttling. This could be added if beneficial.
> > > > > > >
> > > > > > > 2. The throttling values are currently exposed and adjustable in
> > > > > > > /sys/kernel/debug/swiotlb. Should any of this be moved so it is
> > > > > > > visible even without CONFIG_DEBUG_FS?  
> > > > > >
> > > > > > Yes. It should be possible to control the thresholds through
> > > > > > sysctl.  
> > > > >
> > > > > Good point.  I was thinking about creating /sys/kernel/swiotlb, but
> > > > > sysctl is better.  
> > > >
> > > > That still leaves the question where it should go.
> > > >
> > > > Under /proc/sys/kernel? Or should we make a /proc/sys/kernel/dma
> > > > subdirectory to make room for more dma-related controls?  
> > >
> > > I would be good with /proc/sys/kernel/swiotlb (or "dma"). There
> > > are only two entries (high_throttle and low_throttle), but just
> > > dumping everything directly in /proc/sys/kernel doesn't seem like
> > > a good long-term approach.  Even though there are currently a lot
> > > of direct entries in /proc/sys/kernel, that may be historical, and not
> > > changeable due to backwards compatibility requirements.  
> > 
> > I think SWIOTLB is a bit too narrow. How many controls would we add
> > under /proc/sys/kernel/swiotlb? The chances seem higher if we call it
> > /proc/sys/kernel/dma/swiotlb_{low,high}_throttle, and it follows the
> > paths in source code (which are subject to change any time, however).
> > Anyway, I don't want to get into bikeshedding; I'm fine with whatever
> > you send in the end. :-)
> > 
> > BTW those entries directly under /proc/sys/kernel are not all
> > historical. The io_uring_* controls were added just last year, see
> > commit 76d3ccecfa18.
> >   
> 
> Note that there could be multiple instances of the throttle values, since
> a DMA restricted pool has its own struct io_tlb_mem that is separate
> from the default. I wrote the code so that throttling is independently
> applied to a restricted pool as well, though I haven't tested it.

Good point. I didn't think about it.

> So the typical case is that we'll have high and low throttle values for the
> default swiotlb pool, but we could also have high and low throttle
> values for any restricted pools.
> 
> Maybe the /proc pathnames would need to be:
> 
>    /proc/sys/kernel/dma/swiotlb_default/high_throttle
>    /proc/sys/kernel/dma/swiotlb_default/low_throttle
>    /proc/sys/kernel/dma/swiotlb_<rpoolname>/high_throttle
>    /proc/sys/kernel/dma/swiotlb_<rpoolname>/low_throttle

If a subdirectory is needed anyway, then we may ditch the dma
directory idea and place swiotlb subdirectories directly under
/proc/sys/kernel.

> Or we could throw all the throttles directly into the "dma" directory,
> though that makes for fairly long names in lieu of a deeper directory
> structure:
> 
>    /proc/sys/kernel/dma/default_swiotlb_high_throttle
>    /proc/sys/kernel/dma/default_swiotlb_low_throttle
>    /proc/sys/kernel/dma/<rpoolname>_swiotlb_high_throttle
>    /proc/sys/kernel/dma/<rpoolname_>swiotlb_low_throttle
> 
> Thoughts?

I have already said I don't care much as long as the naming and/or
placement is not downright confusing. If the default values are
adjusted, they will end up in a config file under /etc/sysctl.d, and
admins will copy&paste it from Stack Exchange.

I mean, you're probably the most interested person on the planet, so
make a choice, and we'll adapt. ;-)

Petr T

Powered by blists - more mailing lists