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;
Date:   Tue, 28 Mar 2023 16:04:29 +0200
From:   Petr =?UTF-8?B?VGVzYcWZw61r?= <petr@tesarici.cz>
To:     "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Cc:     Christoph Hellwig <hch@infradead.org>, "hch@lst.de" <hch@lst.de>,
        "m.szyprowski@samsung.com" <m.szyprowski@samsung.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        Dexuan Cui <decui@microsoft.com>,
        Tianyu Lan <Tianyu.Lan@microsoft.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] swiotlb: Track and report io_tlb_used high water
 mark in debugfs
Message-ID: <20230328160429.38f58623@meshulam.tesarici.cz>
In-Reply-To: <20230328155017.5636393b@meshulam.tesarici.cz>
References: <1679766790-24629-1-git-send-email-mikelley@microsoft.com>
        <ZCJEAx/G0x6zokPF@infradead.org>
        <BYAPR21MB1688852ED49499249368D939D7889@BYAPR21MB1688.namprd21.prod.outlook.com>
        <20230328155017.5636393b@meshulam.tesarici.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 28 Mar 2023 15:50:17 +0200
Petr Tesa=C5=99=C3=ADk <petr@tesarici.cz> wrote:

> On Tue, 28 Mar 2023 13:12:13 +0000
> "Michael Kelley (LINUX)" <mikelley@microsoft.com> wrote:
>=20
> > From: Christoph Hellwig <hch@infradead.org> Sent: Monday, March 27, 202=
3 6:34 PM =20
> > >=20
> > > On Sat, Mar 25, 2023 at 10:53:10AM -0700, Michael Kelley wrote:   =20
> > > > @@ -659,6 +663,14 @@ static int swiotlb_do_find_slots(struct device=
 *dev, int   =20
> > > area_index,   =20
> > > >  	area->index =3D wrap_area_index(mem, index + nslots);
> > > >  	area->used +=3D nslots;
> > > >  	spin_unlock_irqrestore(&area->lock, flags);
> > > > +
> > > > +	new_used =3D atomic_long_add_return(nslots, &total_used);
> > > > +	old_hiwater =3D atomic_long_read(&used_hiwater);
> > > > +	do {
> > > > +		if (new_used <=3D old_hiwater)
> > > > +			break;
> > > > +	} while (!atomic_long_try_cmpxchg(&used_hiwater, &old_hiwater, ne=
w_used));
> > > > +
> > > >  	return slot_index;   =20
> > >=20
> > > Hmm, so we're right in the swiotlb hot path here and add two new glob=
al
> > > atomics?   =20
> >=20
> > It's only one global atomic, except when the high water mark needs to be
> > bumped.  That results in an initial transient of doing the second global
> > atomic, but then it won't be done unless there's a spike in usage or the
> > high water mark is manually reset to zero.  Of course, there's a similar
> > global atomic subtract when the slots are released.
> >=20
> > Perhaps this accounting should go under #ifdef CONFIG_DEBUGFS?  Or
> > even add a swiotlb-specific debugfs config option to cover all the swio=
tlb
> > debugfs code.  From Petr Tesarik's earlier comments, it sounds like the=
re
> > is interest in additional accounting, such as for fragmentation. =20
>=20
> For my purposes, it does not have to be 100% accurate.

Actually, why are these variables global? There can be multiple
io_tlb_mem instances in the system (one SWIOTLB and multiple restricted
DMA pools). Tracking the usage of restricted DMA pools might be useful,
but summing them up with the SWIOTLB not so much. AFAICS the watermark
should be added to struct io_tlb_mem.

Petr T