Message ID | 1548172644-30862-3-git-send-email-eranbe@mellanox.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | Devlink health updates | expand |
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. > { >- 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. >- .dump_size = 0, >- .dump = NULL, This has 0 relation to the patch. Should be separate. > }; > > #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500 >-- >2.17.1 >
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. 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. > >> { >> - 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. > > > >> - .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 >>
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 >>>
On 1/24/2019 11:08 AM, Jiri Pirko wrote: > 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. This would cause all devlink and mlx5e patches to be altogether when moving to the new API, which I don't like at all. (#2) I think we shall take your approach and do a revert and put it all again on top. > >> >>> >>> >>> >>>> - .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 >>>>
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) { - 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)); - 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, - .dump_size = 0, - .dump = NULL, }; #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500