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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 14 Apr 2017 16:29:10 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Shivappa Vikas <vikas.shivappa@...el.com>
cc:     Vikas Shivappa <vikas.shivappa@...ux.intel.com>, x86@...nel.org,
        LKML <linux-kernel@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
        ravi.v.shankar@...el.com, Tony Luck <tony.luck@...el.com>,
        fenghua.yu@...el.com, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 0/8 V4] x86/intel_rdt: Intel Memory bandwidth
 allocation

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