Date:   Fri, 9 Sep 2016 01:59:49 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Fenghua Yu <fenghua.yu@...el.com>
cc:     "H. Peter Anvin" <h.peter.anvin@...el.com>,
        Ingo Molnar <mingo@...e.hu>, Tony Luck <tony.luck@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>, Borislav Petkov <bp@...e.de>,
        Stephane Eranian <eranian@...gle.com>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        David Carrillo-Cisneros <davidcc@...gle.com>,
        Shaohua Li <shli@...com>,
        Ravi V Shankar <ravi.v.shankar@...el.com>,
        Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
        Sai Prakhya <sai.praneeth.prakhya@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: Re: [PATCH v2 33/33] x86/Makefile: Build intel_rdt_rdtgroup.c

On Thu, 8 Sep 2016, Fenghua Yu wrote:

> Build the user interface file intel_rdt_rdtgroup.c.

> +obj-$(CONFIG_INTEL_RDT)	+= intel_rdt.o intel_rdt_rdtgroup.o intel_rdt_schemata.o

This is garbage. Complete and utter garbage.

First of all this changelog is wrong because this adds not only the user
interface file it finally adds all files.

So up to this point nothing in this series was ever compiled.

And as of patch 13/33 the build is broken when CONFIG_RDT is enabled.

The fact that this whole thing is only ever compiled when the last patch is
applied explains completely why the split up of the patches is a total
nightmare.

Why on earth do you think that 

    Documentation/SubmittingPatches::Section 1::3

does not apply for this work? I can't see any reason why this patch series
would be exempt.

So here is what I want to see:

- Take the combined resulting patch and split it into the following
  sections:

   1) Introduce the Kconfig option

   2) Add intel_rdt.c/h and add it to the Makefile

   3) Add all general code which is unrelated to the user space
      interface/group stuff to intel_rdt.c

      (Split that up if and only if it makes sense)

   4) Add the schedule stuff

   5) Add intel_rdtgroup.c and add it to the Makefile

   6) Add the general filesystem code to it

   7) Add the fork/exit stuff

   8) Add the schema support file

   You might add a few more steps where you think it makes sense, e.g. split
   out kernel parameters or whatever is sensible on its own.

   Don't try to tell me that you need to preserve the original stuff from
   Vikas. It's not useful, it's just annoying and there is no point in
   keeping it. If anyone is interested in this horror he can look it up in
   the LKML archives, but there is no reason to stick that mess into the
   kernel history. You still can mention Vikas in the changelog tags as the
   initial author, but just keeping this horror - which has been even more
   horrified by you - is not an option at all.

- Go through these new patches and make sure that _all_ my review comments
  are addressed. I will make sure they are ....

- Add documentation to the patches where you add the functionality. The
  design documentation is ok as an upfront separate patch.

- Add documentation for locking, protection and lifetime rules.

- Make sure it builds/boots at every step with and without CONFIG_RDT.

But I'm certainly not going through that excercise once more and I'm going to
stop looking at the next series immediately when I notice that there is
still major wreckage left.

I also expect that Reviewed-by tags on the next set of patches are
seriously worth it, which I cannot say for this series sadly.

All I have left to say is:

     yell_WTF(nr_wtf_moments);

I leave the value of the function argument to your imagination.

Please bring it close to zero in the next iteration. It's not rocket
science and if you have questions, I'm (still) happy to help.

Thanks,

	tglx

Powered by blists - more mailing lists