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

[Jiri Pirko <jiri@resnulli.us>]

Mon, Jan 21, 2019 at 02:06:44PM CET, eranbe@mellanox.com wrote:
>
>
>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.

If you have 8 SQs, you should have 8 objects (with subobjects) in an
array.


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