RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Yu, Fenghua]

> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 2:10 PM
> On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> > Here is why this patch behaves like this:
> >
> > This patch and actually first 12 patches are directly from last year's
> > cgroup base CAT patch series. The last year's patch series had gone 16
> > versions already. Because the first 12 patches have been reviewed many
> > times, we keep them untouched (except removing cgroup code in patch 8
> > and some unused cdp code in patch 11) and release other patches on top
> > of the first 12 patches.
> 
> Which is not making the review any simpler. In order to understand the
> modifications I have to go back and page in the original stuff from last year
> once again. So I have to read the original patch first to understand the
> modifications and then get the overall picture of the new stuff. Please fold
> stuff back to the proper places so I can start reviewing this thing under the
> new design idea instead of twisting my brain around two designs.

Ok. I will do that.

> 
> > I fully agree this patch should be split if we want to have a good
> > overall patch series.
> 
> Good.
> 
> Thanks,
> 
>   tglx

RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Yu, Fenghua]

> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 2:10 PM
> On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> > Here is why this patch behaves like this:
> >
> > This patch and actually first 12 patches are directly from last year's
> > cgroup base CAT patch series. The last year's patch series had gone 16
> > versions already. Because the first 12 patches have been reviewed many
> > times, we keep them untouched (except removing cgroup code in patch 8
> > and some unused cdp code in patch 11) and release other patches on top
> > of the first 12 patches.
> 
> Which is not making the review any simpler. In order to understand the
> modifications I have to go back and page in the original stuff from last year
> once again. So I have to read the original patch first to understand the
> modifications and then get the overall picture of the new stuff. Please fold
> stuff back to the proper places so I can start reviewing this thing under the
> new design idea instead of twisting my brain around two designs.

Ok. I will do that.

> 
> > I fully agree this patch should be split if we want to have a good
> > overall patch series.
> 
> Good.
> 
> Thanks,
> 
>   tglx

RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Thomas Gleixner]

On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> Here is why this patch behaves like this:
> 
> This patch and actually first 12 patches are directly from last year's
> cgroup base CAT patch series. The last year's patch series had gone 16
> versions already. Because the first 12 patches have been reviewed many
> times, we keep them untouched (except removing cgroup code in patch 8 and
> some unused cdp code in patch 11) and release other patches on top of the
> first 12 patches.

Which is not making the review any simpler. In order to understand the
modifications I have to go back and page in the original stuff from last year
once again. So I have to read the original patch first to understand the
modifications and then get the overall picture of the new stuff. Please fold
stuff back to the proper places so I can start reviewing this thing under the
new design idea instead of twisting my brain around two designs.
 
> I fully agree this patch should be split if we want to have a good overall
> patch series.

Good.

Thanks,

tglx

RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Thomas Gleixner]

On Wed, 13 Jul 2016, Yu, Fenghua wrote:
> Here is why this patch behaves like this:
> 
> This patch and actually first 12 patches are directly from last year's
> cgroup base CAT patch series. The last year's patch series had gone 16
> versions already. Because the first 12 patches have been reviewed many
> times, we keep them untouched (except removing cgroup code in patch 8 and
> some unused cdp code in patch 11) and release other patches on top of the
> first 12 patches.

Which is not making the review any simpler. In order to understand the
modifications I have to go back and page in the original stuff from last year
once again. So I have to read the original patch first to understand the
modifications and then get the overall picture of the new stuff. Please fold
stuff back to the proper places so I can start reviewing this thing under the
new design idea instead of twisting my brain around two designs.
 
> I fully agree this patch should be split if we want to have a good overall
> patch series.

Good.

Thanks,

tglx

RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Yu, Fenghua]

> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 3:26 AM
> On Tue, 12 Jul 2016, Fenghua Yu wrote:
> 
> Subject: Define CONFIG_INTEL_RDT
> 
> That does not qualify as a proper patch subject
> 
> > From: Vikas Shivappa 
> >
> > CONFIG_INTEL_RDT is defined.
> 
> That tells us what?
> 
> > --- a/arch/x86/include/asm/intel_rdt.h
> > +++ b/arch/x86/include/asm/intel_rdt.h
> > @@ -24,8 +24,16 @@ struct clos_cbm_table {
> >   * on scheduler hot path:
> >   * - This will stay as no-op unless we are running on an Intel SKU
> >   * which supports L3 cache allocation.
> > + * - When support is present and enabled, does not do any
> > + * IA32_PQR_MSR writes until the user starts really using the feature
> > + * ie creates a rdtgroup directory and assigns a cache_mask thats
> > + * different from the root rdtgroup's cache_mask.
> >   * - Caches the per cpu CLOSid values and does the MSR write only
> > - * when a task with a different CLOSid is scheduled in.
> > + * when a task with a different CLOSid is scheduled in. That
> > + * means the task belongs to a different rdtgroup.
> > + * - Closids are allocated so that different rdtgroup directories
> > + * with same cache_mask gets the same CLOSid. This minimizes CLOSids
> > + * used and reduces MSR write frequency.
> 
> How is this and the following changes related to $subject ?

No, this piece of code is not related to $subject.

Here is why this patch behaves like this:

This patch and actually first 12 patches are directly from last year's cgroup 
base CAT patch
series. The last year's patch series had gone 16 versions already. Because
the first 12 patches have been reviewed many times, we keep them untouched
(except removing cgroup code in patch 8 and some unused cdp code in patch 11)
and release other patches on top of the first 12 patches.

I fully agree this patch should be split if we want to have a good overall
patch series.

Thanks.

-Fenghua

RE: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Yu, Fenghua]

> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: Wednesday, July 13, 2016 3:26 AM
> On Tue, 12 Jul 2016, Fenghua Yu wrote:
> 
> Subject: Define CONFIG_INTEL_RDT
> 
> That does not qualify as a proper patch subject
> 
> > From: Vikas Shivappa 
> >
> > CONFIG_INTEL_RDT is defined.
> 
> That tells us what?
> 
> > --- a/arch/x86/include/asm/intel_rdt.h
> > +++ b/arch/x86/include/asm/intel_rdt.h
> > @@ -24,8 +24,16 @@ struct clos_cbm_table {
> >   * on scheduler hot path:
> >   * - This will stay as no-op unless we are running on an Intel SKU
> >   * which supports L3 cache allocation.
> > + * - When support is present and enabled, does not do any
> > + * IA32_PQR_MSR writes until the user starts really using the feature
> > + * ie creates a rdtgroup directory and assigns a cache_mask thats
> > + * different from the root rdtgroup's cache_mask.
> >   * - Caches the per cpu CLOSid values and does the MSR write only
> > - * when a task with a different CLOSid is scheduled in.
> > + * when a task with a different CLOSid is scheduled in. That
> > + * means the task belongs to a different rdtgroup.
> > + * - Closids are allocated so that different rdtgroup directories
> > + * with same cache_mask gets the same CLOSid. This minimizes CLOSids
> > + * used and reduces MSR write frequency.
> 
> How is this and the following changes related to $subject ?

No, this piece of code is not related to $subject.

Here is why this patch behaves like this:

This patch and actually first 12 patches are directly from last year's cgroup 
base CAT patch
series. The last year's patch series had gone 16 versions already. Because
the first 12 patches have been reviewed many times, we keep them untouched
(except removing cgroup code in patch 8 and some unused cdp code in patch 11)
and release other patches on top of the first 12 patches.

I fully agree this patch should be split if we want to have a good overall
patch series.

Thanks.

-Fenghua

Re: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Thomas Gleixner]

On Tue, 12 Jul 2016, Fenghua Yu wrote:

Subject: Define CONFIG_INTEL_RDT

That does not qualify as a proper patch subject

> From: Vikas Shivappa 
> 
> CONFIG_INTEL_RDT is defined. 

That tells us what?

> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -24,8 +24,16 @@ struct clos_cbm_table {
>   * on scheduler hot path:
>   * - This will stay as no-op unless we are running on an Intel SKU
>   * which supports L3 cache allocation.
> + * - When support is present and enabled, does not do any
> + * IA32_PQR_MSR writes until the user starts really using the feature
> + * ie creates a rdtgroup directory and assigns a cache_mask thats
> + * different from the root rdtgroup's cache_mask.
>   * - Caches the per cpu CLOSid values and does the MSR write only
> - * when a task with a different CLOSid is scheduled in.
> + * when a task with a different CLOSid is scheduled in. That
> + * means the task belongs to a different rdtgroup.
> + * - Closids are allocated so that different rdtgroup directories
> + * with same cache_mask gets the same CLOSid. This minimizes CLOSids
> + * used and reduces MSR write frequency.

How is this and the following changes related to $subject ?

Thanks,

tglx

Re: [PATCH 08/32] Define CONFIG_INTEL_RDT

[Thomas Gleixner]

On Tue, 12 Jul 2016, Fenghua Yu wrote:

Subject: Define CONFIG_INTEL_RDT

That does not qualify as a proper patch subject

> From: Vikas Shivappa 
> 
> CONFIG_INTEL_RDT is defined. 

That tells us what?

> --- a/arch/x86/include/asm/intel_rdt.h
> +++ b/arch/x86/include/asm/intel_rdt.h
> @@ -24,8 +24,16 @@ struct clos_cbm_table {
>   * on scheduler hot path:
>   * - This will stay as no-op unless we are running on an Intel SKU
>   * which supports L3 cache allocation.
> + * - When support is present and enabled, does not do any
> + * IA32_PQR_MSR writes until the user starts really using the feature
> + * ie creates a rdtgroup directory and assigns a cache_mask thats
> + * different from the root rdtgroup's cache_mask.
>   * - Caches the per cpu CLOSid values and does the MSR write only
> - * when a task with a different CLOSid is scheduled in.
> + * when a task with a different CLOSid is scheduled in. That
> + * means the task belongs to a different rdtgroup.
> + * - Closids are allocated so that different rdtgroup directories
> + * with same cache_mask gets the same CLOSid. This minimizes CLOSids
> + * used and reduces MSR write frequency.

How is this and the following changes related to $subject ?

Thanks,

tglx