Re: [PATCH for-next 1/2] RDMA/core: Introduce ratelimited ibdev printk functions

[Leon Romanovsky <leon@kernel.org>]

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 <galpress@amazon.com>
> >>>> ---
> >>>>  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.

>
> 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.

Thanks