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

Groups > linux.kernel > #1652182 > unrolled thread

Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

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 2/3] x86/mce/AMD: Define a list_head for threshold blocks  outside the list Borislav Petkov <bp@alien8.de> - 2017-05-28 19:30 +0200
    RE: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks  outside the list "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-05-30 14:50 +0200
      Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks  outside the list Borislav Petkov <bp@alien8.de> - 2017-05-30 16:00 +0200
        RE: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks  outside the list "Ghannam, Yazen" <Yazen.Ghannam@amd.com> - 2017-05-30 16:10 +0200
          Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks  outside the list Borislav Petkov <bp@alien8.de> - 2017-05-30 16:20 +0200

#1652182 — Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> There needs to be a list_head outside of a linked list in order to iterate
> over it and have access to all its elements. This is because the
> list_for_each* macros iterate starting from head->next rather than head.
> 
> Define a list_head for the threshold blocks list in struct threshold_bank

struct threshold_block {

	...

        struct list_head miscj;                 /*
                                                 * List of threshold blocks
                                                 * within a bank.
                                                 */

There's your list_head right there.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [next] | [standalone]

#1653200

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, May 28, 2017 1:22 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Borislav Petkov <bp@suse.de>; Tony Luck
> <tony.luck@intel.com>; x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > There needs to be a list_head outside of a linked list in order to
> > iterate over it and have access to all its elements. This is because
> > the
> > list_for_each* macros iterate starting from head->next rather than head.
> >
> > Define a list_head for the threshold blocks list in struct
> > threshold_bank
> 
> struct threshold_block {
> 
> 	...
> 
>         struct list_head miscj;                 /*
>                                                  * List of threshold blocks
>                                                  * within a bank.
>                                                  */
> 
> There's your list_head right there.
> 

Like I said in the commit message, the list_head needs to be outside the
list to access all the elements using list_for_each*. Otherwise, we won't
get a reference to the "head" element since we iterate starting from
head->next and break when !head.

For example here, we use block 0 as the head and I find that I don't get a
reference to it when using list_for_each* as the code is currently. Am I
doing something wrong?

Thanks,
Yazen

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

#1653253

On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> Like I said in the commit message, the list_head needs to be outside the
> list to access all the elements using list_for_each*. Otherwise, we won't
> get a reference to the "head" element since we iterate starting from
> head->next and break when !head.

I believe the whole hierarchy here is done a bit differently: the list
starts at threshold_bank->blocks which points to the first threshold
block. So when iterating, you need to look at threshold_bank->blocks
first, which is the first element and then traverse the list.

This is basically how the list gets built:

allocate_threshold_blocks:

	...

        if (per_cpu(threshold_banks, cpu)[bank]->blocks)
                list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
        else
                per_cpu(threshold_banks, cpu)[bank]->blocks = b;

per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as
it points to a struct threshold_block and then the ..->blocks->miscj
contains any further threshold blocks present on this bank and we queue
them there if ->blocks is not NULL.

HTH.

-- 
Regards/Gruss,
    Boris.

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

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

#1653259

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, May 30, 2017 9:57 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 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> > Like I said in the commit message, the list_head needs to be outside
> > the list to access all the elements using list_for_each*. Otherwise,
> > we won't get a reference to the "head" element since we iterate
> > starting from
> > head->next and break when !head.
> 
> I believe the whole hierarchy here is done a bit differently: the list starts at
> threshold_bank->blocks which points to the first threshold block. So when
> iterating, you need to look at threshold_bank->blocks first, which is the first
> element and then traverse the list.
> 

Yeah, I thought about doing this in the THR interrupt handler. But then I thought
it might be better to re-define the head so that we could iterate through all of
the elements without having to check the first and iterate the rest.

> This is basically how the list gets built:
> 
> allocate_threshold_blocks:
> 
> 	...
> 
>         if (per_cpu(threshold_banks, cpu)[bank]->blocks)
>                 list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks-
> >miscj);
>         else
>                 per_cpu(threshold_banks, cpu)[bank]->blocks = b;
> 
> per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as it points
> to a struct threshold_block and then the ..->blocks->miscj contains any further
> threshold blocks present on this bank and we queue them there if ->blocks is
> not NULL.
> 

Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Thanks,
Yazen

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

#1653264

On Tue, May 30, 2017 at 02:06:11PM +0000, Ghannam, Yazen wrote:
> Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Nope, I took it already.

-- 
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