From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 09A7D23D for ; Tue, 19 Dec 2017 14:13:55 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id g130so11399640wme.0 for ; Tue, 19 Dec 2017 05:13:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=mto4QFGal9M0M4Y8BaeB7u7t7Amz0IFShWMw02kReeE=; b=ALsQ/J9b3QHB/sClABHMnWLVaafuYKNBxmzUWK4FCXHtWyV9DJ000WQVO2vIMC0AQt U+FGyaMQtlFxWP9HXVL7mauAHf5Le/c2wyXZx7y/p/CjFs+rCYJ7pbaUeb0nf1HKO0Fw pahEFscj4c+/DvMfBkKWf/kNAkoe8AEu/PQGW/eH6OB8jOTQGSY+0ENLpNeo2MKNODUZ dUf8aJBiwdf8XBXsuENlU/5s+KMU0aw5BXOuxI9v2Xlp9KBuA9nBFyOfxIiJAHQqy1D2 xqpaKFxWZ6SD3tbZiXzq1Y7qRU9DlDSTltSNOVlPhTQ2xIK1bwW5g0uPf97fzuecmE+s QINQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=mto4QFGal9M0M4Y8BaeB7u7t7Amz0IFShWMw02kReeE=; b=ZrwO4FeFdqwBMhPl+IFyHApSXoacLC+WmOzj88kR07qk8kBOaKi7oDPriW/8XV2uN9 awVDBxny1ZXr/W5etyYlSy66T9sImW7AyZ2pu7UhIPhM0zDLwg/zyK8ASxMP58GT41Nt NIhvK8Sb2x9Ub/1lAwjaxIYOPShcfCNkPW3y574FLko+M26ukoNmRvLCyRATKEXquWh9 wplpl0xbL1op4ihCsCt19Gq7Wdv123k5fRhCylqoGrRZtJSuUuNZh+9d0aDJlowDnnQr rC0O/Hcc/gYJv2w7TMaOkYmPc3jeF670fKMjAyJgebhx00j+WfQCmLZZvWBg8nfvVQWP qKyA== X-Gm-Message-State: AKGB3mKQAAeJTDvg8RkP+Q9z4/4WNuXPxB1fVHAybf8n/h1DXV9+S8rr +MNKvwHM4oq4NaDqAsLXL2UWY7b4 X-Google-Smtp-Source: ACJfBotuBDfxaHlSNARJ+Oymc3S/swC9XUey4YtyIWpzS1d/W0EIGjiJW0Fpc8hJvorl3HbsvFvjuA== X-Received: by 10.80.190.76 with SMTP id b12mr544706edi.184.1513689235515; Tue, 19 Dec 2017 05:13:55 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r4sm12855319edd.2.2017.12.19.05.13.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 19 Dec 2017 05:13:54 -0800 (PST) Date: Tue, 19 Dec 2017 14:13:43 +0100 From: Adrien Mazarguil To: Thomas Monjalon Cc: dev@dpdk.org, Stephen Hemminger , Ferruh Yigit Message-ID: <20171219131343.GF5377@6wind.com> References: <20171124172132.GW4062@6wind.com> <20171218131751.5c5d9467@xeon-e3> <20171219100117.GA5377@6wind.com> <18024020.yCgE5lJeyx@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <18024020.yCgE5lJeyx@xps> Subject: Re: [dpdk-dev] [PATCH v1 1/3] net/hyperv: introduce MS Hyper-V platform driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Dec 2017 13:13:56 -0000 On Tue, Dec 19, 2017 at 12:15:38PM +0100, Thomas Monjalon wrote: > 19/12/2017 11:01, Adrien Mazarguil: > > On Mon, Dec 18, 2017 at 01:17:51PM -0800, Stephen Hemminger wrote: > > > On Mon, 18 Dec 2017 20:54:16 +0100 > > > Thomas Monjalon wrote: > > > > > > > > > +#endif /* RTE_LIBRTE_HYPERV_DEBUG */ > > > > > > + > > > > > > +#define DEBUG(...) PMD_DRV_LOG(DEBUG, __VA_ARGS__) > > > > > > +#define INFO(...) PMD_DRV_LOG(INFO, __VA_ARGS__) > > > > > > +#define WARN(...) PMD_DRV_LOG(WARNING, __VA_ARGS__) > > > > > > +#define ERROR(...) PMD_DRV_LOG(ERR, __VA_ARGS__) > > > > > > + > > > > > > > > > > Please don't use DEBUG() etc macros. It makes it easier for tools that do > > > > > global updates or scans if all drivers use the same model of PMD_DRV_LOG > > > > > > > > The new standard is to use dynamic logtype. > > > > > > Agree, please use dynamic logging, and also don't redefine new macros like DEBUG/INFO/WARN/ERROR. > > > Instead use PMD_DRV_LOG or equivalent macros. > > > > Wait, the above definitions are only convenience wrappers to PMD_DRV_LOG(), > > itself a wrapper to RTE_LOG(), itself a wrapper to rte_log(), their presence > > is not triggered according to compilation options, did I miss something? > > > > Let me bring back some context from the original patch: > > > > #ifdef RTE_LIBRTE_HYPERV_DEBUG > > > > #define PMD_DRV_LOG(level, ...) \ > > RTE_LOG(level, PMD, \ > > RTE_FMT("%s:%u: %s(): " RTE_FMT_HEAD(__VA_ARGS__,) "\n", \ > > strrchr("/" __FILE__, '/') + 1, \ > > __LINE__, \ > > __func__, \ > > RTE_FMT_TAIL(__VA_ARGS__,))) > > > > #else /* RTE_LIBRTE_HYPERV_DEBUG */ > > > > #define PMD_DRV_LOG(level, ...) \ > > RTE_LOG(level, PMD, \ > > RTE_FMT(RTE_STR(HYPERV_DRIVER) ": " \ > > RTE_FMT_HEAD(__VA_ARGS__,) "\n", \ > > RTE_FMT_TAIL(__VA_ARGS__,))) > > > > #endif /* RTE_LIBRTE_HYPERV_DEBUG */ > > > > #define DEBUG(...) PMD_DRV_LOG(DEBUG, __VA_ARGS__) > > #define INFO(...) PMD_DRV_LOG(INFO, __VA_ARGS__) > > #define WARN(...) PMD_DRV_LOG(WARNING, __VA_ARGS__) > > #define ERROR(...) PMD_DRV_LOG(ERR, __VA_ARGS__) > > > > Enabling RTE_LIBRTE_HYPERV_DEBUG adds file and line information to log > > output, messages are otherwise unaffected by that compilation option. Adding > > this information required some sort of wrapper to avoid needless clutter. > > > > Nothing against outputting file/line information when compiled in debug mode > > right? > > I am not sure __FILE__, __LINE__ and __func__ are so much useful. > The log message should be unique enough. I don't share your opinion. mlx4/mlx5 PMDs output similar information when compiled in debug mode and that proved quite useful during development and when tracking down bugs. Thing is, mere users are not the target audience, it's a development tool that doesn't need to be part of distributed binaries, hence the compilation option. > > > The base rule here is that all drivers should look the same as much > > > as reasonably possible. This makes reviewers of other subsystems more likely > > > to see problems. It also allows for later changes where some developer does a global > > > improvement across many PMD's. > > > > > > Drivers should not be snowflakes, each one is not unique. > > > > Point taken, do you confirm replacing i.e. WARN(...) with > > PMD_DRV_LOG(WARN, ...) and friends is all that's needed? > > You need to remove the compile-time option for DEBUG, > and rely on dynamic log type, thanks to rte_log_register(). OK, I didn't know about rte_log_register() which may explain some of the confusion, I'll add it in v2 then. To summarize what needs to be done for v2: - Call rte_log_register() during init. - Use its return value in place of the second argument to RTE_LOG(). - Replace DEBUG/WARN/INFO/ERROR() wrappers with direct calls to PMD_DRV_LOG() for consistency with other PMDs. - Finally, remove debugging code/information and related compilation option since they're useless to end users. -- Adrien Mazarguil 6WIND