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

From: Shivappa Vikas <vikas.shivappa@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Shivappa Vikas <vikas.shivappa@intel.com>,
	Vikas Shivappa <vikas.shivappa@linux.intel.com>,
	x86@kernel.org, LKML <linux-kernel@vger.kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@kernel.org>,
	ravi.v.shankar@intel.com, Tony Luck <tony.luck@intel.com>,
	fenghua.yu@intel.com, Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth allocation
Date: Fri, 14 Apr 2017 10:52:19 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1704141051161.3411@vshiva-Udesk> (raw)
In-Reply-To: <alpine.DEB.2.20.1704141611290.2327@nanos>

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
>