Re: [PATCH net-next v2 09/11] net/mlx5e: Add TX reporter support

From: Eran Ben Elisha <eranbe@mellanox.com>
To: Jiri Pirko <jiri@resnulli.us>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Jiri Pirko <jiri@mellanox.com>,
	"David S. Miller" <davem@davemloft.net>,
	Ariel Almog <ariela@mellanox.com>, Aya Levin <ayal@mellanox.com>,
	Moshe Shemesh <moshe@mellanox.com>
Subject: Re: [PATCH net-next v2 09/11] net/mlx5e: Add TX reporter support
Date: Mon, 21 Jan 2019 13:06:44 +0000	[thread overview]
Message-ID: <48d40e8e-4dc9-dcfc-6d25-a5b58d9a2419@mellanox.com> (raw)
In-Reply-To: <20190121121154.GF2228@nanopsycho>

On 1/21/2019 2:11 PM, Jiri Pirko wrote:
> Mon, Jan 21, 2019 at 12:32:07PM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/20/2019 1:06 PM, Jiri Pirko wrote:
>>> Thu, Jan 17, 2019 at 10:59:18PM CET, eranbe@mellanox.com wrote:
> 
> [...]
> 
> 	
>>>> +static int
>>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>>>> +					u32 sqn, u8 state, u8 stopped)
>>>> +{
>>>> +	int err, i;
>>>> +	int nest = 0;
>>>> +	char name[20];
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +	nest++;
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +	nest++;
>>>> +
>>>> +	sprintf(name, "SQ 0x%x", sqn);
>>>
>>> No. The whole idea of having the message build up with nested attributes
>>> (json-like) was to avoid things like this. No sprintfs please. If you
>>> want to do sprintf, most of the time you are doing something wrong.
>>
>> I wanted that each SQ object will have a unique name. But I can merge
>> the sqn into its attributes instead.
> 
> Should be an array.

Every SQ shall hold it's own properties. I don't see why all SQs shall 
be held under one array, this array do not provide any additional 
info/order.

In addition, it is better to keep main preliminary objects up to the 
size of one netlink SKB, otherwise it will be impossible for the devlink 
to prepare this one big object as skb fragments, and also to re-assmble 
them in user space as driver intended them to be.

If all SWs will be under one big array, under one big object, we will 
hit this corner case.

> 
> 
>>
>>>
>>>
>>>> +	err = devlink_health_buffer_put_object_name(buffer, name);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +	nest++;
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +	nest++;
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +	nest++;
>>>> +
>>>> +	err = devlink_health_buffer_put_object_name(buffer, "HW state");
>>>> +	if (err)
>>>> +		goto buffer_error;
>>>> +
>>>> +	err = devlink_health_buffer_nest_start(buffer,
>>>> +					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>
>>> How could you put an object name and don't start nesting? It looks
>>> implicit to me.
>>
>> I will add some helper functions that you could review. Just keep in
>> mind the implicit nest start must come with implicit nest end, so it
>> won't fit into every use...
> 
> You can do just object_start(), object_finish() or something like that.

ack.

Better to have also helpers that start and close their nests on one 
shot. like pair_name_value or pair_name_value_array.

Also, The json is too powerful to close it inside few wrappers, we must 
give the basic blocks as well for nontraditional structures.

> 
> [...]
> 