Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API

[Jiri Pirko <jiri@resnulli.us>]

Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>
>
>On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>>> reporter used devlink health buffers API. Which will soon be depracated.
>>> Modify the reporter to use the new devlink msg API.
>>>
>>> The actual set of the new diagnose function will be done later in the
>>> downstream patch, only when devlink will actually expose a new diagnose
>>> operation that uses the new API.
>>>
>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>> ---
>>> .../mellanox/mlx5/core/en/reporter_tx.c       | 123 ++++--------------
>>> 1 file changed, 26 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> index d9675afbb924..fc92850c214a 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
>>> }
>>>
>>> static int
>>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
>>> 					u32 sqn, u8 state, u8 stopped)
>> 
>> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
>> 
>stopped is the reply from netif_xmit_stopped, and it is a bool.

If it is bool, you should add it into message as NLA_FLAG, not NLA_U8


>state is the HW state of the SQ.
>it is defined in the ifc file:
>enum {
>         MLX5_SQC_STATE_RST  = 0x0,
>         MLX5_SQC_STATE_RDY  = 0x1,
>         MLX5_SQC_STATE_ERR  = 0x3,
>};
>I could have translated it into strings, but I though it would be fine 
>to leave it as is.

It's fine as u8

>
>> 
>>> {
>>> -	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);
>>> -	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;
>>> +	int err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>> +	err = devlink_msg_object_start(msg_ctx, "SQ");
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_value_u8(buffer, state);
>>> +	err = devlink_msg_object_start(msg_ctx, NULL);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -
>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>>> -	nest--;
>>> -
>>> -	devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>>> -	nest--;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>>> +					      sizeof(sqn), NLA_U32);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_object_name(buffer, "stopped");
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>>> +					      sizeof(state), NLA_U8);
>>> 	if (err)
>>> -		goto buffer_error;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_nest_start(buffer,
>>> -					       DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>> +	err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>>> +					      sizeof(stopped), NLA_U8);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -	nest++;
>>> +		return err;
>>>
>>> -	err = devlink_health_buffer_put_value_u8(buffer, stopped);
>>> +	err = devlink_msg_object_end(msg_ctx, NULL);
>>> 	if (err)
>>> -		goto buffer_error;
>>> -
>>> -	for (i = 0; i < nest; i++)
>>> -		devlink_health_buffer_nest_end(buffer);
>>> -
>>> -	return 0;
>>> +		return err;
>>>
>>> -buffer_error:
>>> -	for (i = 0; i < nest; i++)
>>> -		devlink_health_buffer_nest_cancel(buffer);
>>> +	err = devlink_msg_object_end(msg_ctx, "SQ");
>>> 	return err;
>>> }
>>>
>>> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> -				      struct devlink_health_buffer **buffers_array,
>>> -				      unsigned int buffer_size,
>>> -				      unsigned int num_buffers)
>>> +				      struct devlink_msg_ctx *msg_ctx)
>>> {
>>> 	struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>>> -	unsigned int buff = 0;
>>> -	int i = 0, err = 0;
>>> -
>>> -	if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>>> -		return -ENOMEM;
>>> +	int i, err = 0;
>>>
>>> 	mutex_lock(&priv->state_lock);
>>>
>>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> 		return 0;
>>> 	}
>>>
>>> -	while (i < priv->channels.num * priv->channels.params.num_tc) {
>>> +	for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>>> +	     i++) {
>>> 		struct mlx5e_txqsq *sq = priv->txq2sq[i];
>>> 		u8 state;
>>>
>>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> 		if (err)
>>> 			break;
>>>
>>> -		err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>>> -							      sq->sqn, state,
>>> +		err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>>> +							      state,
>>> 							      netif_xmit_stopped(sq->txq));
>> 
>> This should be an array. On SQ entry : one member of array.
>> 
>> So if you want to change it, you need to do this in 2 patches. One API,
>> one the actual layout. :/
>> 
>> 
>> 
>>> -		if (err) {
>>> -			if (++buff == num_buffers)
>>> -				break;
>>> -		} else {
>>> -			i++;
>>> -		}
>>> +		if (err)
>>> +			break;
>>> 	}
>>>
>>> 	mutex_unlock(&priv->state_lock);
>>> @@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>> 		.name = "TX",
>>> 		.recover = mlx5e_tx_reporter_recover,
>>> -		.diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>>> -				 MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>>> -		.diagnose = mlx5e_tx_reporter_diagnose,
>> 
>> So you change the callback, remove it so the dissection is broken.
>
>This is needed in order to have this patch compiled.

You have to figure it out without breakage. This is not the way to
do this.

>
>> 
>> 
>> 
>>> -		.dump_size = 0,
>>> -		.dump = NULL,
>> 
>> 
>> This has 0 relation to the patch. Should be separate.
>
>ack.
>
>> 
>> 
>>> };
>>>
>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>> -- 
>>> 2.17.1
>>>