From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Mon, 23 Apr 2018 15:03:17 +0200 From: Greg Kroah-Hartman To: NeilBrown Cc: James Simmons , Oleg Drokin , Andreas Dilger , Linux Kernel Mailing List , Lustre Development List Subject: Re: [PATCH 1/6] staging: lustre: move stack-check macros to libcfs_debug.h Message-ID: <20180423130317.GA17153@kroah.com> References: <152383910760.23409.2327082725637657049.stgit@noble> <152383935730.23409.6748888065027051683.stgit@noble> <87a7u1s1fi.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a7u1s1fi.fsf@notabene.neil.brown.name> User-Agent: Mutt/1.9.5 (2018-04-13) X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Wed, Apr 18, 2018 at 12:17:37PM +1000, NeilBrown wrote: > On Mon, Apr 16 2018, James Simmons wrote: > > >> CDEBUG_STACK() and CHECK_STACK() are macros to help with > >> debugging, so move them from > >> drivers/staging/lustre/include/linux/libcfs/linux/libcfs.h > >> to > >> drivers/staging/lustre/include/linux/libcfs/libcfs_debug.h > >> > >> This seems a more fitting location, and is a step towards > >> removing linux/libcfs.h and simplifying the include file structure. > > > > Nak. Currently the lustre client always enables debugging but that > > shouldn't be the case. What we do need is the able to turn off the > > crazy debugging stuff. In the development branch of lustre it is > > done with CDEBUG_ENABLED. We need something like that in Kconfig > > much like we have CONFIG_LUSTRE_DEBUG_EXPENSIVE_CHECK. Since we like > > to be able to turn that off this should be moved to just after > > LIBCFS_DEBUG_MSG_DATA_DECL. Then from CHECK_STACK down to CWARN() > > it can be build out. When CDEBUG_ENABLED is disabled CDEBUG_LIMIT > > would be empty. > > So why, exactly, is this an argument to justify a NAK? > Are you just saying that the code I moved into libcfs_debug.h should be > moved to somewhere a bit later in the file? > That can easily be done when it is needed. It isn't needed now so why > insist on it? > > Each patch should do one thing and make clear forward progress. This > patch gets rid of an unnecessary file and brings related code together. > I think that qualifies. I agree, this just deletes an unused file, it changes no functionality at all. Now applied. greg k-h