| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Dec 2023 20:46:07 -0800
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Peter Zijlstra <peterz@...radead.org>,
LKML <linux-kernel@...r.kernel.org>, X86 Kernel <x86@...nel.org>,
iommu@...ts.linux.dev, Lu Baolu <baolu.lu@...ux.intel.com>,
kvm@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>,
Joerg Roedel <joro@...tes.org>,
"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
Ingo Molnar <mingo@...hat.com>,
Raj Ashok <ashok.raj@...el.com>,
"Tian, Kevin" <kevin.tian@...el.com>, maz@...nel.org,
seanjc@...gle.com, Robin Murphy <robin.murphy@....com>,
jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH RFC 09/13] x86/irq: Install posted MSI notification
handler
Hi Thomas,
On Wed, 06 Dec 2023 20:50:24 +0100, Thomas Gleixner <tglx@...utronix.de>
wrote:
> On Wed, Nov 15 2023 at 13:56, Peter Zijlstra wrote:
> >
> > Would it not make more sense to write things something like:
> >
> > bool handle_pending_pir()
> > {
> > bool handled = false;
> > u64 pir_copy[4];
> >
> > for (i = 0; i < 4; i++) {
> > if (!pid-pir_l[i]) {
> > pir_copy[i] = 0;
> > continue;
> > }
> >
> > pir_copy[i] = arch_xchg(&pir->pir_l[i], 0);
> > handled |= true;
> > }
> >
> > if (!handled)
> > return handled;
> >
> > for_each_set_bit()
> > ....
> >
> > return handled.
> > }
>
> I don't understand what the whole copy business is about. It's
> absolutely not required.
>
> static bool handle_pending_pir(unsigned long *pir)
> {
> unsigned int idx, vec;
> bool handled = false;
> unsigned long pend;
>
> for (idx = 0; offs < 4; idx++) {
> if (!pir[idx])
> continue;
> pend = arch_xchg(pir + idx, 0);
> for_each_set_bit(vec, &pend, 64)
> call_irq_handler(vec + idx * 64, NULL);
> handled = true;
> }
> return handled;
> }
>
My thinking is the following:
The PIR cache line is contended by between CPU and IOMMU, where CPU can
access PIR much faster. Nevertheless, when IOMMU does atomic swap of the
PID (PIR included), L1 cache gets evicted. Subsequent CPU read or xchg will
deal with invalid cold cache.
By making a copy of PIR as quickly as possible and clearing PIR with xchg,
we minimized the chance that IOMMU does atomic swap in the middle.
Therefore, having less L1D misses.
In the code above, it does read, xchg, and call_irq_handler() in a loop
to handle the 4 64bit PIR bits at a time. IOMMU has a greater chance to do
atomic xchg on the PIR cache line while doing call_irq_handler(). Therefore,
it causes more L1D misses.
I might be missing something?
I tested the two versions below with my DSA memory fill test and measured
DMA bandwidth and perf cache misses:
#ifdef NO_PIR_COPY
static __always_inline inline bool handle_pending_pir(u64 *pir, struct pt_regs *regs)
{
int i, vec;
bool handled = false;
unsigned long pending;
for (i = 0; i < 4; i++) {
if (!pir[i])
continue;
pending = arch_xchg(pir + i, 0);
for_each_set_bit(vec, &pending, 64)
call_irq_handler(i * 64 + vec, regs);
handled = true;
}
return handled;
}
#else
static __always_inline inline bool handle_pending_pir(u64 *pir, struct pt_regs *regs)
{
int i, vec = FIRST_EXTERNAL_VECTOR;
bool handled = false;
unsigned long pir_copy[4];
for (i = 0; i < 4; i++)
pir_copy[i] = pir[i];
for (i = 0; i < 4; i++) {
if (!pir_copy[i])
continue;
pir_copy[i] = arch_xchg(pir, 0);
handled = true;
}
if (handled) {
for_each_set_bit_from(vec, pir_copy, FIRST_SYSTEM_VECTOR)
call_irq_handler(vec, regs);
}
return handled;
}
#endif
DEFINE_IDTENTRY_SYSVEC(sysvec_posted_msi_notification)
{
struct pt_regs *old_regs = set_irq_regs(regs);
struct pi_desc *pid;
int i = 0;
pid = this_cpu_ptr(&posted_interrupt_desc);
inc_irq_stat(posted_msi_notification_count);
irq_enter();
while (i++ < MAX_POSTED_MSI_COALESCING_LOOP) {
if (!handle_pending_pir(pid->pir64, regs))
break;
}
/*
* Clear outstanding notification bit to allow new IRQ notifications,
* do this last to maximize the window of interrupt coalescing.
*/
pi_clear_on(pid);
/*
* There could be a race of PI notification and the clearing of ON bit,
* process PIR bits one last time such that handling the new interrupts
* are not delayed until the next IRQ.
*/
handle_pending_pir(pid->pir64, regs);
apic_eoi();
irq_exit();
set_irq_regs(old_regs);
}
Without PIR copy:
DMA memfill bandwidth: 4.944 Gbps
Performance counter stats for './run_intr.sh 512 30':
77,313,298,506 L1-dcache-loads (79.98%)
8,279,458 L1-dcache-load-misses # 0.01% of all L1-dcache accesses (80.03%)
41,654,221,245 L1-dcache-stores (80.01%)
10,476 LLC-load-misses # 0.31% of all LL-cache accesses (79.99%)
3,332,748 LLC-loads (80.00%)
30.212055434 seconds time elapsed
0.002149000 seconds user
30.183292000 seconds sys
With PIR copy:
DMA memfill bandwidth: 5.029 Gbps
Performance counter stats for './run_intr.sh 512 30':
78,327,247,423 L1-dcache-loads (80.01%)
7,762,311 L1-dcache-load-misses # 0.01% of all L1-dcache accesses (80.01%)
42,203,221,466 L1-dcache-stores (79.99%)
23,691 LLC-load-misses # 0.67% of all LL-cache accesses (80.01%)
3,561,890 LLC-loads (80.00%)
30.201065706 seconds time elapsed
0.005950000 seconds user
30.167885000 seconds sys
> No?
>
> > sysvec_posted_blah_blah()
> > {
> > bool done = false;
> > bool handled;
> >
> > for (;;) {
> > handled = handle_pending_pir();
> > if (done)
> > break;
> > if (!handled || ++loops > MAX_LOOPS) {
>
> That does one loop too many. Should be ++loops == MAX_LOOPS. No?
>
> > pi_clear_on(pid);
> > /* once more after clear_on */
> > done = true;
> > }
> > }
> > }
> >
> >
> > Hmm?
>
> I think that can be done less convoluted.
>
> {
> struct pi_desc *pid = this_cpu_ptr(&posted_interrupt_desc);
> struct pt_regs *old_regs = set_irq_regs(regs);
> int loops;
>
> for (loops = 0;;) {
> bool handled = handle_pending_pir((unsigned
> long)pid->pir);
>
> if (++loops > MAX_LOOPS)
> break;
>
> if (!handled || loops == MAX_LOOPS) {
> pi_clear_on(pid);
> /* Break the loop after handle_pending_pir()! */
> loops = MAX_LOOPS;
> }
> }
>
> ...
> set_irq_regs(old_regs);
> }
>
> Hmm? :)
Thanks,
Jacob
Powered by blists - more mailing lists