From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81469C32751 for ; Wed, 31 Jul 2019 13:33:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 49729206A2 for ; Wed, 31 Jul 2019 13:33:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564579995; bh=atHXoef4Y/8S89bUuRCjoi5QNyRRm9cuQwF5onZ1G3c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=ROecPZ7e9xi8QJ8qnxhla6aPKPOjEU8bGFO8S557LvAz50g6lQCA0jzUUSbpDBvnF iYBIDawTNrLck8Rant1Cki50QcK6wNt/8qu9Zd3x1jFOL7ACtyabrrCl9OYBPB25gh mYiRBSO7QHRQTH/5ZXb0Ir+vlPUeZNnBxbFuv2LQ= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726339AbfGaNdO (ORCPT ); Wed, 31 Jul 2019 09:33:14 -0400 Received: from mail.kernel.org ([198.145.29.99]:55294 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725209AbfGaNdO (ORCPT ); Wed, 31 Jul 2019 09:33:14 -0400 Received: from localhost (unknown [77.137.115.125]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 6FE29206A2; Wed, 31 Jul 2019 13:33:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564579993; bh=atHXoef4Y/8S89bUuRCjoi5QNyRRm9cuQwF5onZ1G3c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TnV0xkhQuGHMhmMBMjoUDNXRMKd7jp+qSHFe5HHKOLNGfWMTDRmAIC7ftQIlt9VZP 7KAHinoA5xA0S6SPoHVarklj6Z6ZV6HXakLoUtEPyK8qCyqM8v+xmd6olPoTuaXjkI ETIvLuVI0etnGCtPsKuQ6pLrx1DqLdPhjZl/88qs= Date: Wed, 31 Jul 2019 16:33:09 +0300 From: Leon Romanovsky To: Gal Pressman Cc: Jason Gunthorpe , Doug Ledford , linux-rdma@vger.kernel.org Subject: Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions Message-ID: <20190731133309.GW4878@mtr-leonro.mtl.com> References: <20190730151834.70993-1-galpress@amazon.com> <20190730151834.70993-2-galpress@amazon.com> <20190730154148.GG4878@mtr-leonro.mtl.com> <20190731074109.GL4878@mtr-leonro.mtl.com> <20190731114609.GS4878@mtr-leonro.mtl.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.0 (2019-05-25) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Wed, Jul 31, 2019 at 03:56:55PM +0300, Gal Pressman wrote: > On 31/07/2019 14:46, Leon Romanovsky wrote: > > On Wed, Jul 31, 2019 at 01:51:05PM +0300, Gal Pressman wrote: > >> On 31/07/2019 10:41, Leon Romanovsky wrote: > >>> On Wed, Jul 31, 2019 at 10:22:42AM +0300, Gal Pressman wrote: > >>>> On 30/07/2019 18:41, Leon Romanovsky wrote: > >>>>> On Tue, Jul 30, 2019 at 06:18:33PM +0300, Gal Pressman wrote: > >>>>>> Add ratelimited helpers to the ibdev_* printk functions. > >>>>>> Implementation inspired by counterpart dev_*_ratelimited functions. > >>>>>> > >>>>>> Signed-off-by: Gal Pressman > >>>>>> --- > >>>>>> include/rdma/ib_verbs.h | 51 +++++++++++++++++++++++++++++++++++++++++ > >>>>>> 1 file changed, 51 insertions(+) > >>>>>> > >>>>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h > >>>>>> index c5f8a9f17063..356e6a105366 100644 > >>>>>> --- a/include/rdma/ib_verbs.h > >>>>>> +++ b/include/rdma/ib_verbs.h > >>>>>> @@ -107,6 +107,57 @@ static inline > >>>>>> void ibdev_dbg(const struct ib_device *ibdev, const char *format, ...) {} > >>>>>> #endif > >>>>>> > >>>>>> +#define ibdev_level_ratelimited(ibdev_level, ibdev, fmt, ...) \ > >>>>>> +do { \ > >>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \ > >>>>>> + DEFAULT_RATELIMIT_INTERVAL, \ > >>>>>> + DEFAULT_RATELIMIT_BURST); \ > >>>>>> + if (__ratelimit(&_rs)) \ > >>>>>> + ibdev_level(ibdev, fmt, ##__VA_ARGS__); \ > >>>>>> +} while (0) > >>>>>> + > >>>>>> +#define ibdev_emerg_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_emerg, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_alert_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_alert, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_crit_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_crit, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_err_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_err, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_warn_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_warn, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_notice_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_notice, ibdev, fmt, ##__VA_ARGS__) > >>>>>> +#define ibdev_info_ratelimited(ibdev, fmt, ...) \ > >>>>>> + ibdev_level_ratelimited(ibdev_info, ibdev, fmt, ##__VA_ARGS__) > >>>>>> + > >>>>>> +#if defined(CONFIG_DYNAMIC_DEBUG) > >>>>>> +/* descriptor check is first to prevent flooding with "callbacks suppressed" */ > >>>>>> +#define ibdev_dbg_ratelimited(ibdev, fmt, ...) \ > >>>>>> +do { \ > >>>>>> + static DEFINE_RATELIMIT_STATE(_rs, \ > >>>>>> + DEFAULT_RATELIMIT_INTERVAL, \ > >>>>>> + DEFAULT_RATELIMIT_BURST); \ > >>>>>> + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ > >>>>>> + if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) \ > >>>>>> + __dynamic_ibdev_dbg(&descriptor, ibdev, fmt, \ > >>>>>> + ##__VA_ARGS__); \ > >>>>>> +} while (0) > >>>>>> +#elif defined(DEBUG) > >>>>> > >>>>> When will you see this CONFIG_DEBUG set? I suspect only in private > >>>>> out-of-tree builds which we are not really care. Also I can't imagine > >>>>> system with this CONFIG_DEBUG and without CONFIG_DYNAMIC_DEBUG. > >>>> > >>>> This is the common way to handle debug prints, see: > >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/printk.h#L331 > >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/device.h#L1493 > >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/netdevice.h#L4743 > >>>> https://elixir.bootlin.com/linux/v5.2.1/source/include/linux/net.h#L266 > >>> > >>> I'm more interested to know the real usage of this copy/paste and > >>> understand if it makes sense for drivers/infiniband/* or not. > >>> > >>> Not everything in netdev is great and worth to borrow. > >> > >> DEBUG exists since the first commit in the tree, and is used in various parts of > >> the kernel (mlx5 as well). Do you think it should be removed from the kernel? > > > > It is gradually removed when it is spotted, I'll send a patch for mlx5 now. > > Was there an on-list discussion regarding removal of DEBUG usage? Can you please > share a link? Sorry, but no, I didn't know that I need to save all discussions I see in the mailing lists. > If so, I agree the DEBUG part should be removed. > > > > >> > >> Regarding combination of both, I don't think DEBUG is related to > >> CONFIG_DYNAMIC_DEBUG. DEBUG is a generic debug flag (not necessarily to prints) > >> while CONFIG_DYNAMIC_DEBUG is specific to the dynamic debug prints infrastructure. > > > > I know exactly what DEBUG and CONFIG_DYNAMIC_DEBUG mean, but I'm > > asking YOU to provide us real and in-tree scenario where DEBUG will > > exists and CONFIG_DYNAMIC_DEBUG won't. > > What's any of this has to do with in-tree? This code and defines are part of the > tree. > > The use case doesn't matter, it's a valid permutation. Is there anything that > stops a user from building the kernel this way? Like everything else, nothing stops from you to do any modifications to the source code, before you will build. We are talking about in-tree builds and distro kernels. Thanks