Change amd_set_l3_disable_slot to check for block interference
cpumask, and avoid the IPI if set.
Also change wbinvd_on_cpu to use smp_call_function_single_fail.
Signed-off-by: Marcelo Tosatti <[email protected]>
Index: linux-isolation/arch/x86/include/asm/smp.h
===================================================================
--- linux-isolation.orig/arch/x86/include/asm/smp.h
+++ linux-isolation/arch/x86/include/asm/smp.h
@@ -118,7 +118,7 @@ int native_cpu_disable(void);
void __noreturn hlt_play_dead(void);
void native_play_dead(void);
void play_dead_common(void);
-void wbinvd_on_cpu(int cpu);
+int wbinvd_on_cpu(int cpu);
int wbinvd_on_all_cpus(void);
void smp_kick_mwait_play_dead(void);
Index: linux-isolation/arch/x86/kernel/cpu/cacheinfo.c
===================================================================
--- linux-isolation.orig/arch/x86/kernel/cpu/cacheinfo.c
+++ linux-isolation/arch/x86/kernel/cpu/cacheinfo.c
@@ -17,6 +17,7 @@
#include <linux/sysfs.h>
#include <linux/pci.h>
#include <linux/stop_machine.h>
+#include <linux/sched/isolation.h>
#include <asm/cpufeature.h>
#include <asm/cacheinfo.h>
@@ -396,6 +397,7 @@ static void amd_l3_disable_index(struct
* disable index in all 4 subcaches
*/
for (i = 0; i < 4; i++) {
+ int ret;
u32 reg = idx | (i << 20);
if (!nb->l3_cache.subcaches[i])
@@ -409,6 +411,7 @@ static void amd_l3_disable_index(struct
* is not sufficient.
*/
ret = wbinvd_on_cpu(cpu);
+ WARN_ON(ret == -EPERM);
reg |= BIT(31);
pci_write_config_dword(nb->misc, 0x1BC + slot * 4, reg);
@@ -428,7 +431,7 @@ static void amd_l3_disable_index(struct
static int amd_set_l3_disable_slot(struct amd_northbridge *nb, int cpu,
unsigned slot, unsigned long index)
{
- int ret = 0;
+ int idx, ret = 0;
/* check if @slot is already used or the index is already disabled */
ret = amd_get_l3_disable_slot(nb, slot);
@@ -442,7 +445,15 @@ static int amd_set_l3_disable_slot(struc
if (index == amd_get_l3_disable_slot(nb, !slot))
return -EEXIST;
- amd_l3_disable_index(nb, cpu, slot, index);
+ ret = 0;
+ idx = block_interf_srcu_read_lock();
+
+ if (block_interf_cpu(cpu))
+ ret = -EPERM;
+ else
+ amd_l3_disable_index(nb, cpu, slot, index);
+
+ block_interf_srcu_read_unlock(idx);
return 0;
}
Index: linux-isolation/arch/x86/lib/cache-smp.c
===================================================================
--- linux-isolation.orig/arch/x86/lib/cache-smp.c
+++ linux-isolation/arch/x86/lib/cache-smp.c
@@ -7,9 +7,9 @@ static void __wbinvd(void *dummy)
wbinvd();
}
-void wbinvd_on_cpu(int cpu)
+int wbinvd_on_cpu(int cpu)
{
- smp_call_function_single(cpu, __wbinvd, NULL, 1);
+ return smp_call_function_single_fail(cpu, __wbinvd, NULL, 1);
}
EXPORT_SYMBOL(wbinvd_on_cpu);
On Tue, Feb 06 2024 at 15:49, Marcelo Tosatti wrote:
> @@ -396,6 +397,7 @@ static void amd_l3_disable_index(struct
> * disable index in all 4 subcaches
> */
> for (i = 0; i < 4; i++) {
> + int ret;
> u32 reg = idx | (i << 20);
>
> if (!nb->l3_cache.subcaches[i])
> @@ -409,6 +411,7 @@ static void amd_l3_disable_index(struct
> * is not sufficient.
> */
> ret = wbinvd_on_cpu(cpu);
> + WARN_ON(ret == -EPERM);
What? You create inconsistent state here.
> - amd_l3_disable_index(nb, cpu, slot, index);
> + ret = 0;
> + idx = block_interf_srcu_read_lock();
> +
> + if (block_interf_cpu(cpu))
> + ret = -EPERM;
> + else
> + amd_l3_disable_index(nb, cpu, slot, index);
> +
> + block_interf_srcu_read_unlock(idx);
Again. This is a root only operation.
This whole patch series is just voodoo programming with zero
justification for the mess it creates.
Thanks,
tglx
.
On Wed, Feb 07, 2024 at 01:41:36PM +0100, Thomas Gleixner wrote:
> On Tue, Feb 06 2024 at 15:49, Marcelo Tosatti wrote:
> > @@ -396,6 +397,7 @@ static void amd_l3_disable_index(struct
> > * disable index in all 4 subcaches
> > */
> > for (i = 0; i < 4; i++) {
> > + int ret;
> > u32 reg = idx | (i << 20);
> >
> > if (!nb->l3_cache.subcaches[i])
> > @@ -409,6 +411,7 @@ static void amd_l3_disable_index(struct
> > * is not sufficient.
> > */
> > ret = wbinvd_on_cpu(cpu);
> > + WARN_ON(ret == -EPERM);
>
> What? You create inconsistent state here.
That should not happen, since we checked for
+ idx = block_interf_srcu_read_lock();
+
+ if (block_interf_cpu(cpu))
+ ret = -EPERM;
Earlier.
Thus the WARN_ON (hum, can change to BUG_ON...).
> > - amd_l3_disable_index(nb, cpu, slot, index);
> > + ret = 0;
> > + idx = block_interf_srcu_read_lock();
> > +
> > + if (block_interf_cpu(cpu))
> > + ret = -EPERM;
> > + else
> > + amd_l3_disable_index(nb, cpu, slot, index);
> > +
> > + block_interf_srcu_read_unlock(idx);
>
> Again. This is a root only operation.
>
> This whole patch series is just voodoo programming with zero
> justification for the mess it creates.
>
> Thanks,
>
> tglx
Its not really voodoo programming: its simply returning errors to
userspace if a CPU is set in a particular cpumask.
Do you have a better idea? (i am in the process of converting more
users).
Can improve on the patchset quality.
Ok, the justification is as follows. Today, there is a reactive approach
to interruptions to isolated CPUs:
1) Run Linux+latency sensitive application on isolated CPU.
2) Wait for IPI or other interruption to happen on that isolated CPU,
which breaks the application.
3) Remove that interruption to the isolated CPU.
This is (for a class of IPIs), an active approach, where those IPIs are
not permitted to interrupt the latency sensitive applications.
https://iot-analytics.com/soft-plc-industrial-innovators-dilemma/
"Hard PLCs (a market in which incumbent vendors dominate) have
historically addressed most of the needs of the existing / high end
market, such as high reliability, fast cycle times and, perhaps most
importantly, the ability of existing workforce to support and maintain
the systems. Soft PLCs, on the other hand, initially addressed the needs
of new / lower end customers by providing more flexible,
non-deterministic control solutions often at a fraction of the cost of
similar hard PLCs. Since entering the market in the 90’s, soft PLCs have
rapidly become more performant thanks to advances in virtualization
technologies, real-time Linux operating systems and more powerful edge
computing hardware, thus moving up the y-axis (product performance) in
the chart above."
On Wed, Feb 07, 2024 at 10:10:41AM -0300, Marcelo Tosatti wrote:
> On Wed, Feb 07, 2024 at 01:41:36PM +0100, Thomas Gleixner wrote:
> > On Tue, Feb 06 2024 at 15:49, Marcelo Tosatti wrote:
> > > @@ -396,6 +397,7 @@ static void amd_l3_disable_index(struct
> > > * disable index in all 4 subcaches
> > > */
> > > for (i = 0; i < 4; i++) {
> > > + int ret;
> > > u32 reg = idx | (i << 20);
> > >
> > > if (!nb->l3_cache.subcaches[i])
> > > @@ -409,6 +411,7 @@ static void amd_l3_disable_index(struct
> > > * is not sufficient.
> > > */
> > > ret = wbinvd_on_cpu(cpu);
> > > + WARN_ON(ret == -EPERM);
> >
> > What? You create inconsistent state here.
>
> That should not happen, since we checked for
>
> + idx = block_interf_srcu_read_lock();
> +
> + if (block_interf_cpu(cpu))
> + ret = -EPERM;
>
> Earlier.
>
> Thus the WARN_ON (hum, can change to BUG_ON...).
>
> > > - amd_l3_disable_index(nb, cpu, slot, index);
> > > + ret = 0;
> > > + idx = block_interf_srcu_read_lock();
> > > +
> > > + if (block_interf_cpu(cpu))
> > > + ret = -EPERM;
> > > + else
> > > + amd_l3_disable_index(nb, cpu, slot, index);
> > > +
> > > + block_interf_srcu_read_unlock(idx);
> >
> > Again. This is a root only operation.
> >
> > This whole patch series is just voodoo programming with zero
> > justification for the mess it creates.
BTW, this converted less than 17% callers, so this is why early
feedback is useful.
Thanks!