Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665881 > unrolled thread

Re: [PATCH] x86/mce: Always save severity in machine_check_poll

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/mce: Always save severity in machine_check_poll Borislav Petkov <bp@alien8.de> - 2017-06-14 16:30 +0200
    RE: [PATCH] x86/mce: Always save severity in machine_check_poll "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-06-16 17:00 +0200
      Re: [PATCH] x86/mce: Always save severity in machine_check_poll Borislav Petkov <bp@alien8.de> - 2017-06-19 18:50 +0200

#1665881 — Re: [PATCH] x86/mce: Always save severity in machine_check_poll

On Mon, Jun 12, 2017 at 11:54:06AM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Remove code that was used to decide whether to schedule work. The decision

???

I'm missing a *lot* of background in order to understand what that sentence
means.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [next] | [standalone]

#1667816

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Borislav Petkov
> Sent: Wednesday, June 14, 2017 10:21 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] x86/mce: Always save severity in machine_check_poll
> 
> On Mon, Jun 12, 2017 at 11:54:06AM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > Remove code that was used to decide whether to schedule work. The
> > decision
> 
> ???
> 
> I'm missing a *lot* of background in order to understand what that sentence
> means.
> 

The code block being removed here was added in the following commit to decide
whether or not to schedule work.

fa92c58 x86, mce: Support memory error recovery for both UCNA and Deferred error in machine_check_poll

The following commit based the decision to schedule work on if we have a usable
address and made this decision later in machine_check_poll().

8b38937b x86/mce: Do not enter deferred errors into the generic pool twice

Then the following commit removed m.usable_addr from the code block.

c0ec382 x86/RAS: Remove mce.usable_addr

So now this code block just decides whether or not to save the severity. We can
remove this block, since the original purpose of this code (to schedule work) is no
longer happening.

Tony has a concern that some notifiers may assume that the severity being
set means that the error is a memory error. As far as I can tell, the only notifier
that uses severity is the SRAO notifier and it doesn't make an assumption.

We schedule work if we want to log the error or if we have a usable address.
So there's no reason not to save the severity anymore.

Thanks,
Yazen

[toc] | [prev] | [next] | [standalone]

#1669482

On Fri, Jun 16, 2017 at 02:49:58PM +0000, Ghannam, Yazen wrote:
> The code block being removed here was added in the following commit to decide
> whether or not to schedule work.
> 
> fa92c58 x86, mce: Support memory error recovery for both UCNA and Deferred error in machine_check_poll

Are you sure?

That commit makes the code log poison memory errors which have a valid
address. The work being scheduled is only an implementation detail for
how we're doing the logging and calling to the userspace handlers to
start consuming.

Btw, do

$ git config --replace-all core.abbrev 12

to set your commit abbreviation length to 12 so that there's no
ambiguity for the recent years. :)

> The following commit based the decision to schedule work on if we have a usable
> address and made this decision later in machine_check_poll().

> 8b38937b x86/mce: Do not enter deferred errors into the generic pool twice

Yes, because we were logging them twice.

> Then the following commit removed m.usable_addr from the code block.
> 
> c0ec382 x86/RAS: Remove mce.usable_addr

That's just a cleanup.

> So now this code block just decides whether or not to save the severity. We can
> remove this block, since the original purpose of this code (to schedule work) is no
> longer happening.

Not really. The original purpose of this code was to log deferred errors
with AddrV set - the work scheduling is just an implementation detail,
as I mentioned above.

> Tony has a concern that some notifiers may assume that the severity being
> set means that the error is a memory error. As far as I can tell, the only notifier
> that uses severity is the SRAO notifier and it doesn't make an assumption.
> 
> We schedule work if we want to log the error or if we have a usable address.
> So there's no reason not to save the severity anymore.

Just forget the work scheduling - it is only a marginal implementation
thing - you only want to say that we want to log the severity since we
already have it. And the most important aspect in your v2 commit message
is explaining *why* we want the severity. And yes, it is possible to
simply say, this is a cleanup and we can just as well always save it
because it is a valid reason. And it makes sense too.

All I'm trying to remind you of, is, you should explain in your commit
message what the issue is and *why* you're doing the change. When the
first sentence of your commit message is "Remove code that was used
to decide whether to schedule work." I'm starting to scratch my head,
wondering, what the actual problem is and what you're trying to fix.

Ok?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web