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

Groups > linux.kernel > #1622604 > unrolled thread

Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth allocation

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 0/8 V4] x86/intel_rdt: Intel Memory bandwidth  allocation Shivappa Vikas <vikas.shivappa@intel.com> - 2017-04-13 01:00 +0200
    Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth  allocation Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 01:40 +0200
      Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth  allocation Thomas Gleixner <tglx@linutronix.de> - 2017-04-14 16:30 +0200
        Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth  allocation Shivappa Vikas <vikas.shivappa@linux.intel.com> - 2017-04-14 20:00 +0200
          Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth  allocation Shivappa Vikas <vikas.shivappa@linux.intel.com> - 2017-04-15 02:20 +0200

#1622604 — Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth allocation

Hello Thomas,

This series has minor changes with respect to V3 addressing all your comments.
Was wondering if there was any feedback or if we still have a chance for 4.12.

Thanks,
Vikas

On Fri, 7 Apr 2017, Vikas Shivappa wrote:

> Sending another version of  MBA patch series with changes to V3 version
> as per Thomas feedback here:
> https://marc.info/?l=linux-kernel&m=149125664024881
>
> Changes:
> - Fixed the wrong names in struct document for rdt_domain
> - Changed the mba and cache ctrl values to have seperate structs and put
> them in a union in rdt_resource structure.
>
> patch applies on tip x86/cpus
>
> [PATCH 1/8] Documentation, x86: Intel Memory bandwidth allocation
> [PATCH 2/8] x86/intel_rdt/mba: Generalize the naming to get ready for
> [PATCH 3/8] x86/intel_rdt/mba: Memory b/w allocation feature detect
> [PATCH 4/8] x86/intel_rct/mba: Add MBA structures and initialize MBA
> [PATCH 5/8] x86/intel_rdt: Prep to add info files for MBA
> [PATCH 6/8] x86/intel_rdt/mba: Add info directory files for MBA
> [PATCH 7/8] x86/intel_rdt: Prep to add schemata file for MBA
> [PATCH 8/8] x86/intel_rdt/mba: Add schemata file support for MBA
>

[toc] | [next] | [standalone]

#1622615

On Wed, 12 Apr 2017, Shivappa Vikas wrote:
> This series has minor changes with respect to V3 addressing all your comments.
> Was wondering if there was any feedback or if we still have a chance for 4.12.

It's on my radar and should make it, unless there is some major hickup.

Thanks,

	tglx

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

#1623693

On Thu, 13 Apr 2017, Thomas Gleixner wrote:

> On Wed, 12 Apr 2017, Shivappa Vikas wrote:
> > This series has minor changes with respect to V3 addressing all your comments.
> > Was wondering if there was any feedback or if we still have a chance for 4.12.
> 
> It's on my radar and should make it, unless there is some major hickup.

To be honest, I almost dropped it because as usual you cobbled it together
in a hurry just to get it out the door.

I asked for putting the CBM and MBA related data into seperate structs and
make a anon union of them in struct rdt_resource. Instead you went and made
it a anon union of anon structs, so you did not have to change anything
else in the code. What's the point of this? That's a completely useless
exercise and even worse than the data lump which was there before.

I also asked several times in the past to split preparatory stuff from new
stuff. No, the msr_update crap comes in one go. You introduce an update
function and instead of replacing _ALL_ loops you keep one and then fix it
up in some completely unrelated patch.

The ordering of the new struct members was also completely random along
with the kernel doc comments not being aligned.

As a bonus, you reimplemented roundup() open coded in the bandwidth
validation function.

Instead of wasting my time for another round of review and another delivery
of half baken crap, I fixed it up myself. The result is pushed out to
tip/x86/cpu.

Please do the following:

 1) Verify that it still works as I have no hardware to test it. Once you
    confirmed, it's going to show up in -next. So please do that ASAP,
    i.e. yesterday.

 2) Go through the patches one by one and compare it to your own to figure
    out yourself how it should be done. Next time, I'm simply going to drop
    such crap whether that makes it miss the merge window or not.

Yours grumpy

      tglx

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

#1623817

On Fri, 14 Apr 2017, Thomas Gleixner wrote:

> On Thu, 13 Apr 2017, Thomas Gleixner wrote:
>
>> On Wed, 12 Apr 2017, Shivappa Vikas wrote:
>>> This series has minor changes with respect to V3 addressing all your comments.
>>> Was wondering if there was any feedback or if we still have a chance for 4.12.
>>
>> It's on my radar and should make it, unless there is some major hickup.
>
> To be honest, I almost dropped it because as usual you cobbled it together
> in a hurry just to get it out the door.
>
> I asked for putting the CBM and MBA related data into seperate structs and
> make a anon union of them in struct rdt_resource. Instead you went and made
> it a anon union of anon structs, so you did not have to change anything
> else in the code. What's the point of this? That's a completely useless
> exercise and even worse than the data lump which was there before.
>
> I also asked several times in the past to split preparatory stuff from new
> stuff. No, the msr_update crap comes in one go. You introduce an update
> function and instead of replacing _ALL_ loops you keep one and then fix it
> up in some completely unrelated patch.
>
> The ordering of the new struct members was also completely random along
> with the kernel doc comments not being aligned.
>
> As a bonus, you reimplemented roundup() open coded in the bandwidth
> validation function.
>
> Instead of wasting my time for another round of review and another delivery
> of half baken crap, I fixed it up myself. The result is pushed out to
> tip/x86/cpu.
>
> Please do the following:
>
> 1) Verify that it still works as I have no hardware to test it. Once you
>    confirmed, it's going to show up in -next. So please do that ASAP,
>    i.e. yesterday.
>
> 2) Go through the patches one by one and compare it to your own to figure
>    out yourself how it should be done. Next time, I'm simply going to drop
>    such crap whether that makes it miss the merge window or not.

Ok doing the testing now. Will update soon.
Also will followup with the type of changes and implement the same convention in 
the future patches.

Thanks,
Vikas

>
> Yours grumpy
>
>      tglx
>

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

#1623967

On Fri, 14 Apr 2017, Shivappa Vikas wrote:

>
>
> On Fri, 14 Apr 2017, Thomas Gleixner wrote:
>> 
>> Please do the following:
>> 
>> 1) Verify that it still works as I have no hardware to test it. Once you
>>    confirmed, it's going to show up in -next. So please do that ASAP,
>>    i.e. yesterday.
>> 
>> 2) Go through the patches one by one and compare it to your own to figure
>>    out yourself how it should be done. Next time, I'm simply going to drop
>>    such crap whether that makes it miss the merge window or not.
>
> Ok doing the testing now. Will update soon.
> Also will followup with the type of changes and implement the same convention 
> in the future patches.

All your changes are tested to be fine when used on the hardware and function 
as before. Although we did discover some minor parsing issues (which existed in 
the version i sent originally..) and will send a fix on top of 
current tip/x86/cpu soon.

Thanks,
Vikas

>
> Thanks,
> Vikas
>
>> 
>> Yours grumpy
>>
>>      tglx
>> 
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web