Re: [PATCH v18 04/19] EDAC: Add memory repair control feature

[Borislav Petkov <bp@alien8.de>]

On Thu, Jan 09, 2025 at 06:34:48PM +0000, Jonathan Cameron wrote:
> Today you can.  Seems we are talking cross purposes.
> 
> I'm confused. I thought your proposal was for "bank" attribute to present an
> allowed range on read.
> "bank" attribute is currently written to and read back as the value of the bank on which
> to conduct a repair.  Maybe this disconnect is down to the fact max_ and min_
> attributes should have been marked as RO in the docs. They aren't controls,
> just presentation of limits to userspace.
> 
> Was intent a separate bank_range type attribute rather than max_bank, min_bank?

I don't know - I'm just throwing ideas out there. You could do:

cat /sys/.../bank

and that gives you

[<low> <current_value> <high>]

So you have all the needed information. Dunno if this would be abusing sysfs
rules too much tho.

> > 
> > > In at least the CXL case I'm fairly sure most of them are not discoverable.
> > > Until you see errors you have no idea what the memory topology is.  
> > 
> > Ok.
> > 
> > > For that you'd need to have a path to read back what happened.  
> > 
> > So how is this scrubbing going to work? You get an error, you parse it for all
:> > the attributes and you go and write those attributes into the scrub interface
> > and it starts scrubbing?
> 
> Repair not scrubbing. They are different things we should keep separate,
> scrub corrects the value, if it can, but doesn't change the underlying memory to
> new memory cells to avoid repeated errors. Replacing scrub with repair 
> (which I think was the intent here)...

Really?

So how is scrubbing defined for CXL? You read memory, do ECC check on it,
report any potential errors but write back the *original* wrong value?!

I thought the point of scrubbing is to repair it while at it too...

> You get error records that describe the error seen in hardware, write back the
> values into this interface and tell it to repair the memory.  This is not
> necessarily a synchronous or immediate thing - instead typically based on
> trend analysis.

This is just silly: I'm scrubbing, I found an error, I should simply fix it
while at it. Why would I need an additional command to repair it?!

> As an example, the decision might be that bit of ram threw up 3 errors
> over a month including multiple system reboots (for other reasons) and
> that is over some threshold so we use a spare memory line to replace it.

Right.

> Short answer, it needs to be very smart and there isn't a case of one size
> fits all - hence suggested approach of making it a user space problem.

Making it a userspace problem is pretty much always a sign that the hw design
failed.

> Given in the systems being considered here, software is triggering the repair,
> we want to allow for policy in the decision. 

Right, you can leave a high-level decision to userspace: repair only when
idle, repair only non-correctable errors, blabla but exposing every single
aspect of every single error... meh.

> In simple cases we could push that policy into the kernel e.g. just repair
> the moment we see an error record.
> 
> These repair resources are very limited in number, so immediately repairing
> may a bad idea. We want to build up a history of errors before making
> such a decision.  That can be done in kernel. 

Yap, we are doing this now:

drivers/ras/cec.c

Userspace involvement is minimal, if at all. It is mostly controlling the
parameters of the leaky bucket.

> The decision to repair memory is heavily influenced by policy and time considerations
> against device resource constraints.
> 
> Some options that are hard to do in kernel.
> 
> 1. Typical asynchronous error report for a corrected error.
> 
>    Tells us memory had an error (perhaps from a scrubbing engine on the device
>    running checks). No need to take action immediately. Instead build up more data
>    over time and if lots of errors occur make decision to repair as no we are sure it
>    is worth doing rather than a single random event. We may tune scrubbing engines
>    to check this memory more frequently and adjust our data analysis to take that
>    into account for setting thresholds etc.

See above.

What happens when your daemon dies and loses all that collected data?

> 2. Soft repair across boots.  We are actually storing the error records, then only
>    applying the fix on reboot before using the memory - so maintaining a list
>    of bad memory and saving it to a file to read back on boot. We could provide
>    another kernel interface to get this info and reinject it after reboot instead
>    of doing it in userspace but that is another ABI to design.

We did something similar recently: drivers/ras/amd/fmpm.c. It basically
"replays" errors from persistent storage as that memory cannot be replaced.

> 3. Complex policy across fleets.  A lot of work is going on around prediction techniques
>    that may change the local policy on each node dependent on the overall reliability
>    patterns of a particular batch of devices and local characteristics, service guarantees
>    etc. If it is hard repair, then once you've run out you need schedule an engineer
>    out to replace the DIMM. All complex inputs to the decision.

You probably could say here: "repair or report when this and that." or
"offline page and report error" and similar high-level decisions by leaving
the details to the kernel instead of looking at every possible error in
userspace and returning back to the kernel to state your decision.

> Similar cases like CPU offlining on repeated errors are done in userspace (e.g.
> RAS Daemon) for similar reasons of long term data gathering and potentially
> complex algorithms.
>   
> > 
> > > Ok. Then can we just drop the range discoverability entirely or we go with
> > > your suggestion and do not support read back of what has been
> > > requested but instead have the reads return a range if known or "" /
> > > return -EONOTSUPP if simply not known?  
> > 
> > Probably.
> 
> Too many options in the above paragraph so just to check...  Probably to which?
> If it's a separate attribute from the one we write the control so then
> we do what is already done here and don't present the interface at all if
> the range isn't discoverable.

Probably means I still don't get a warm and fuzzy feeling about this design.
As I've noted above.

> Ok. Best path is drop the available range support then (so no min_ max_ or
> anything to replace them for now).
> 
> Added bonus is we don't have to rush this conversation and can make sure we
> come to the right solution driven by use cases.

Yap, that sounds like a prudent idea.

What I'm trying to say, basically, is, this interface through sysfs is
a *lot* of attributes, there's no clear cut use case where we can judge how
useful it is and as I alluded to above, I really think that you should leave
the high-level decisions to userspace and let the kernel do its job.

This'll make your interface a lot simpler.

And if you really need to control every single aspect of scrubbing in
userspace, then you can always come later with proper design and use case.

But again, I really think you should keep as much recovery logic in the kernel
and as automatic as possible. Only when you really really need user input,
only then you should allow it...

I hope I'm making sense here...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette