Re: [PATCH rdma-core 03/27] mlx5: Enable debug functionality for vfio

From: Gal Pressman <galpress@amazon.com>
To: Yishai Hadas <yishaih@nvidia.com>, Leon Romanovsky <leon@kernel.org>
Cc: <linux-rdma@vger.kernel.org>, <jgg@nvidia.com>,
	<maorg@nvidia.com>, <markzhang@nvidia.com>, <edwards@nvidia.com>,
	"Leybovich, Yossi" <sleybo@amazon.com>
Subject: Re: [PATCH rdma-core 03/27] mlx5: Enable debug functionality for vfio
Date: Wed, 21 Jul 2021 11:51:18 +0300	[thread overview]
Message-ID: <f1dc0691-d9b0-3dec-d815-41694706bf6e@amazon.com> (raw)
In-Reply-To: <36f3d02d-e70a-dc8e-0dbd-07fad437c2c4@nvidia.com>

On 21/07/2021 10:58, Yishai Hadas wrote:
> On 7/21/2021 10:05 AM, Gal Pressman wrote:
>> On 20/07/2021 17:57, Yishai Hadas wrote:
>>> On 7/20/2021 3:27 PM, Leon Romanovsky wrote:
>>>> On Tue, Jul 20, 2021 at 12:27:46PM +0300, Yishai Hadas wrote:
>>>>> On 7/20/2021 11:51 AM, Leon Romanovsky wrote:
>>>>>> On Tue, Jul 20, 2021 at 11:16:23AM +0300, Yishai Hadas wrote:
>>>>>>> From: Maor Gottlieb <maorg@nvidia.com>
>>>>>>>
>>>>>>> Usage will be in next patches.
>>>>>>>
>>>>>>> Signed-off-by: Maor Gottlieb <maorg@nvidia.com>
>>>>>>> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
>>>>>>> ---
>>>>>>>     providers/mlx5/mlx5.c | 28 ++++++++++++++--------------
>>>>>>>     providers/mlx5/mlx5.h |  4 ++++
>>>>>>>     2 files changed, 18 insertions(+), 14 deletions(-)
>>>>>> Probably, this patch will be needed to be changed after
>>>>>> "Verbs logging API" PR https://github.com/linux-rdma/rdma-core/pull/1030
>>>>>>
>>>>>> Thanks
>>>>> Well, not really, this patch just reorganizes things inside mlx5 for code
>>>>> sharing.
>>>> After Gal's PR, I expect to see all mlx5 file/debug logic gone except
>>>> some minimal conversion logic to support old legacy variables.
>>>>
>>>> All that get_env_... code will go.
>>>>
>>>> Thanks
>>>>
>>> Looking on current VERBs logging PR, it doesn't give a clean path conversion
>>> from mlx5.
>>>
>>> For example it missed a debug granularity per object (e.g. QP, CQ, etc.) , in
>>> addition it doesn't define a driver specific options as we have today in mlx5
>>> (e.g. MLX5_DBG_DR).
>>>
>>> I believe that this should be added before going forward with the logging PR to
>>> enable a clean transition later on.
>>>
>>> The transition of mlx5 should preserve current API/semantics (ENV, etc.) and is
>>> an orthogonal task.
>> Yishai, if you have any more concerns please share them in the PR.. The
>> discussion there is going on for a while and you've been quiet so I assumed
>> you're fine with it.
>>
>> I disagree about needing to support everything that exists in mlx5 today, the
>> purpose of the generic mechanism is to unify the environment variables, driver
>> specific options do the opposite. IMO masking out a few prints isn't worth the
>> divergence.
> 
> 
> The options in mlx5 gives more granularity and supports vendor specific options,
> this may be needed down the road by other vendors as well.
> 
> If we come with a new API need to consider such options in the general case.
> 
> NP, I'll comment in the logging PR as well.
> 
>>
>> If the mlx5 provider has backwards compatibility issues it doesn't necessarily
>> have to use this API, but we can at least covert most existing providers and all
>> future providers that wish to support such functionality in a unified way.
> 
> 
> The point was that current suggestion doesn't allow a clean transition for mlx5,
> so we won't use it unless the API will give a matching alternative.
> 
>> BTW, why even insist on having backwards compatibility here? Do you actually
>> have useful code that relies on debug environment variables?
> 
> Logging options (env, mask, etc.) are kind of API which need to be preserved,
> it's used in the field for debug purposes, no reason to loose granularity and
> trace.

It's used in the field by *people*, which unlike legacy code can learn to use
the new debug env variables, they don't need backwards compatibility (the same
as users of debugfs).
I addressed the granularity issue in the PR.