* [Intel-wired-lan] [PATCH iwl-next v2 1/6] checkpatch: don't complain on _Generic() use
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 2/6] devlink: add devlink_fmsg_put() macro Mateusz Polchlopek
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Przemek Kitszel, Wojciech Drewek,
Simon Horman, Mateusz Polchlopek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Improve CamelCase recognition logic to avoid reporting on
_Generic() use.
Other C keywords, such as _Bool, are intentionally omitted, as those
should be rather avoided in new source code.
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
scripts/checkpatch.pl | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b812210b412..c4a087d325d4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5840,6 +5840,8 @@ sub process {
#CamelCase
if ($var !~ /^$Constant$/ &&
$var =~ /[A-Z][a-z]|[a-z][A-Z]/ &&
+#Ignore C keywords
+ $var !~ /^_Generic$/ &&
#Ignore some autogenerated defines and enum values
$var !~ /^(?:[A-Z]+_){1,5}[A-Z]{1,3}[a-z]/ &&
#Ignore Page<foo> variants
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Intel-wired-lan] [PATCH iwl-next v2 2/6] devlink: add devlink_fmsg_put() macro
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 1/6] checkpatch: don't complain on _Generic() use Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter Mateusz Polchlopek
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Przemek Kitszel, Wojciech Drewek,
Simon Horman, Mateusz Polchlopek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Add devlink_fmsg_put() that dispatches based on the type
of the value to put, example: bool -> devlink_fmsg_bool_pair_put().
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
include/net/devlink.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index db5eff6cb60f..85739bb731c1 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1261,6 +1261,17 @@ enum devlink_trap_group_generic_id {
.min_burst = _min_burst, \
}
+#define devlink_fmsg_put(fmsg, name, value) ( \
+ _Generic((value), \
+ bool : devlink_fmsg_bool_pair_put, \
+ u8 : devlink_fmsg_u8_pair_put, \
+ u16 : devlink_fmsg_u32_pair_put, \
+ u32 : devlink_fmsg_u32_pair_put, \
+ u64 : devlink_fmsg_u64_pair_put, \
+ char * : devlink_fmsg_string_pair_put, \
+ const char * : devlink_fmsg_string_pair_put) \
+ (fmsg, name, (value)))
+
enum {
/* device supports reload operations */
DEVLINK_F_RELOAD = 1UL << 0,
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 1/6] checkpatch: don't complain on _Generic() use Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 2/6] devlink: add devlink_fmsg_put() macro Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-14 14:23 ` Jakub Kicinski
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 4/6] ice: print ethtool stats as part of " Mateusz Polchlopek
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Przemek Kitszel, Igor Bagnucki,
Wojciech Drewek, Mateusz Polchlopek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Add Tx hang devlink health reporter, see struct ice_tx_hang_event to see
what is reported.
Subsequent commits will extend it by more info, for now it dumps
descriptors with little metadata.
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
drivers/net/ethernet/intel/ice/Makefile | 1 +
.../intel/ice/devlink/devlink_health.c | 178 ++++++++++++++++++
.../intel/ice/devlink/devlink_health.h | 34 ++++
drivers/net/ethernet/intel/ice/ice.h | 2 +
drivers/net/ethernet/intel/ice/ice_main.c | 11 +-
5 files changed, 223 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/intel/ice/devlink/devlink_health.c
create mode 100644 drivers/net/ethernet/intel/ice/devlink/devlink_health.h
diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
index 3307d551f431..f2baba82480c 100644
--- a/drivers/net/ethernet/intel/ice/Makefile
+++ b/drivers/net/ethernet/intel/ice/Makefile
@@ -33,6 +33,7 @@ ice-y := ice_main.o \
ice_idc.o \
devlink/devlink.o \
devlink/devlink_port.o \
+ devlink/devlink_health.o \
ice_sf_eth.o \
ice_sf_vsi_vlan_ops.o \
ice_ddp.o \
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
new file mode 100644
index 000000000000..41f203cbdf6a
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Intel Corporation. */
+
+#include "devlink_health.h"
+#include "ice.h"
+
+#define ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, obj, name) \
+ devlink_fmsg_put(fmsg, #name, (obj)->name)
+
+/**
+ * ice_devlink_health_report - boilerplate to call given @reporter
+ *
+ * @reporter: devlink health reporter to call, do nothing on NULL
+ * @msg: message to pass up, "event name" is fine
+ * @priv_ctx: typically some event struct
+ */
+static void ice_devlink_health_report(struct devlink_health_reporter *reporter,
+ const char *msg, void *priv_ctx)
+{
+ int err;
+
+ if (!reporter)
+ return;
+
+ err = devlink_health_report(reporter, msg, priv_ctx);
+ if (err) {
+ struct ice_pf *pf = devlink_health_reporter_priv(reporter);
+
+ dev_err(ice_pf_to_dev(pf),
+ "failed to report %s via devlink health, err %d\n",
+ msg, err);
+ }
+}
+
+/**
+ * ice_fmsg_put_ptr - put hex value of pointer into fmsg
+ *
+ * @fmsg: devlink fmsg under construction
+ * @name: name to pass
+ * @ptr: 64 bit value to print as hex and put into fmsg
+ */
+static void ice_fmsg_put_ptr(struct devlink_fmsg *fmsg, const char *name,
+ void *ptr)
+{
+ char buf[sizeof(ptr) * 3];
+
+ sprintf(buf, "%p", ptr);
+ devlink_fmsg_put(fmsg, name, buf);
+}
+
+struct ice_tx_hang_event {
+ u32 head;
+ u32 intr;
+ u16 vsi_num;
+ u16 queue;
+ u16 next_to_clean;
+ u16 next_to_use;
+ struct ice_tx_ring *tx_ring;
+};
+
+static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
+ struct devlink_fmsg *fmsg, void *priv_ctx,
+ struct netlink_ext_ack *extack)
+{
+ struct ice_tx_hang_event *event = priv_ctx;
+
+ devlink_fmsg_obj_nest_start(fmsg);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, intr);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, vsi_num);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, queue);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, next_to_clean);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, next_to_use);
+ devlink_fmsg_put(fmsg, "irq-mapping", event->tx_ring->q_vector->name);
+ ice_fmsg_put_ptr(fmsg, "desc-ptr", event->tx_ring->desc);
+ ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)(long)event->tx_ring->dma);
+ devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
+ (event->tx_ring->count * sizeof(struct ice_tx_desc)));
+ devlink_fmsg_obj_nest_end(fmsg);
+
+ return 0;
+}
+
+void ice_report_tx_hang(struct ice_pf *pf, struct ice_tx_ring *tx_ring,
+ u16 vsi_num, u32 head, u32 intr)
+{
+ struct ice_tx_hang_event ev = {
+ .head = head,
+ .intr = intr,
+ .vsi_num = vsi_num,
+ .queue = tx_ring->q_index,
+ .next_to_clean = tx_ring->next_to_clean,
+ .next_to_use = tx_ring->next_to_use,
+ .tx_ring = tx_ring,
+ };
+
+ ice_devlink_health_report(pf->health_reporters.tx_hang, "Tx hang", &ev);
+}
+
+static struct devlink_health_reporter *
+ice_init_devlink_rep(struct ice_pf *pf,
+ const struct devlink_health_reporter_ops *ops)
+{
+ struct devlink *devlink = priv_to_devlink(pf);
+ struct devlink_health_reporter *rep;
+ const u64 graceful_period = 0;
+
+ rep = devl_health_reporter_create(devlink, ops, graceful_period, pf);
+ if (IS_ERR(rep)) {
+ struct device *dev = ice_pf_to_dev(pf);
+
+ dev_err(dev, "failed to create devlink %s health report er",
+ ops->name);
+ return NULL;
+ }
+ return rep;
+}
+
+#define ICE_DEFINE_HEALTH_REPORTER_OPS(_name) \
+ static const struct devlink_health_reporter_ops ice_ ## _name ## _reporter_ops = { \
+ .name = #_name, \
+ .dump = ice_ ## _name ## _reporter_dump, \
+}
+
+ICE_DEFINE_HEALTH_REPORTER_OPS(tx_hang);
+
+/**
+ * ice_health_init - allocate and init all ice devlink health reporters and
+ * accompanied data
+ *
+ * @pf: PF struct
+ */
+void ice_health_init(struct ice_pf *pf)
+{
+ struct ice_health *reps = &pf->health_reporters;
+
+ reps->tx_hang = ice_init_devlink_rep(pf, &ice_tx_hang_reporter_ops);
+}
+
+/**
+ * ice_deinit_devl_reporter - destroy given devlink health reporter
+ * @reporter: reporter to destroy
+ */
+static void ice_deinit_devl_reporter(struct devlink_health_reporter *reporter)
+{
+ if (reporter)
+ devl_health_reporter_destroy(reporter);
+}
+
+/**
+ * ice_health_deinit - deallocate all ice devlink health reporters and
+ * accompanied data
+ *
+ * @pf: PF struct
+ */
+void ice_health_deinit(struct ice_pf *pf)
+{
+ ice_deinit_devl_reporter(pf->health_reporters.tx_hang);
+}
+
+static
+void ice_health_assign_healthy_state(struct devlink_health_reporter *reporter)
+{
+ if (reporter)
+ devlink_health_reporter_state_update(reporter,
+ DEVLINK_HEALTH_REPORTER_STATE_HEALTHY);
+}
+
+/**
+ * ice_health_clear - clear devlink health issues after a reset
+ * @pf: the PF device structure
+ *
+ * Mark the PF in healthy state again after a reset has completed.
+ */
+void ice_health_clear(struct ice_pf *pf)
+{
+ ice_health_assign_healthy_state(pf->health_reporters.tx_hang);
+}
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.h b/drivers/net/ethernet/intel/ice/devlink/devlink_health.h
new file mode 100644
index 000000000000..984b8f9f56d4
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2024, Intel Corporation. */
+
+#ifndef _DEVLINK_HEALTH_H_
+#define _DEVLINK_HEALTH_H_
+
+#include <linux/types.h>
+
+/**
+ * DOC: devlink_health.h
+ *
+ * This header file stores everything that is needed for broadly understood
+ * devlink health mechanism for ice driver.
+ */
+
+struct ice_pf;
+struct ice_tx_ring;
+
+/**
+ * struct ice_health - stores ice devlink health reporters and accompanied data
+ * @tx_hang: devlink health reporter for tx_hang event
+ */
+struct ice_health {
+ struct devlink_health_reporter *tx_hang;
+};
+
+void ice_health_init(struct ice_pf *pf);
+void ice_health_deinit(struct ice_pf *pf);
+void ice_health_clear(struct ice_pf *pf);
+
+void ice_report_tx_hang(struct ice_pf *pf, struct ice_tx_ring *tx_ring,
+ u16 vsi_num, u32 head, u32 intr);
+
+#endif /* _DEVLINK_HEALTH_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 0046684004ff..d2f2ed2d4bfa 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -78,6 +78,7 @@
#include "ice_irq.h"
#include "ice_dpll.h"
#include "ice_adapter.h"
+#include "devlink/devlink_health.h"
#define ICE_BAR0 0
#define ICE_REQ_DESC_MULTIPLE 32
@@ -667,6 +668,7 @@ struct ice_pf {
struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
struct ice_dplls dplls;
struct device *hwmon_dev;
+ struct ice_health health_reporters;
};
extern struct workqueue_struct *ice_lag_wq;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index e2990993b16f..1fae7d34f2e4 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -5052,6 +5052,7 @@ static int ice_init_devlink(struct ice_pf *pf)
return err;
ice_devlink_init_regions(pf);
+ ice_health_init(pf);
ice_devlink_register(pf);
return 0;
@@ -5060,6 +5061,7 @@ static int ice_init_devlink(struct ice_pf *pf)
static void ice_deinit_devlink(struct ice_pf *pf)
{
ice_devlink_unregister(pf);
+ ice_health_deinit(pf);
ice_devlink_destroy_regions(pf);
ice_devlink_unregister_params(pf);
}
@@ -7743,6 +7745,8 @@ static void ice_rebuild(struct ice_pf *pf, enum ice_reset_req reset_type)
/* if we get here, reset flow is successful */
clear_bit(ICE_RESET_FAILED, pf->state);
+ ice_health_clear(pf);
+
ice_plug_aux_dev(pf);
if (ice_is_feature_supported(pf, ICE_F_SRIOV_LAG))
ice_lag_rebuild(pf);
@@ -8230,16 +8234,17 @@ void ice_tx_timeout(struct net_device *netdev, unsigned int txqueue)
if (tx_ring) {
struct ice_hw *hw = &pf->hw;
- u32 head, val = 0;
+ u32 head, intr = 0;
head = FIELD_GET(QTX_COMM_HEAD_HEAD_M,
rd32(hw, QTX_COMM_HEAD(vsi->txq_map[txqueue])));
/* Read interrupt register */
- val = rd32(hw, GLINT_DYN_CTL(tx_ring->q_vector->reg_idx));
+ intr = rd32(hw, GLINT_DYN_CTL(tx_ring->q_vector->reg_idx));
netdev_info(netdev, "tx_timeout: VSI_num: %d, Q %u, NTC: 0x%x, HW_HEAD: 0x%x, NTU: 0x%x, INT: 0x%x\n",
vsi->vsi_num, txqueue, tx_ring->next_to_clean,
- head, tx_ring->next_to_use, val);
+ head, tx_ring->next_to_use, intr);
+ ice_report_tx_hang(pf, tx_ring, vsi->vsi_num, head, intr);
}
pf->tx_timeout_last_recovery = jiffies;
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter Mateusz Polchlopek
@ 2024-07-14 14:23 ` Jakub Kicinski
2024-07-22 9:23 ` Mateusz Polchlopek
0 siblings, 1 reply; 13+ messages in thread
From: Jakub Kicinski @ 2024-07-14 14:23 UTC (permalink / raw)
To: Mateusz Polchlopek
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Igor Bagnucki, Wojciech Drewek
On Fri, 12 Jul 2024 05:32:48 -0400 Mateusz Polchlopek wrote:
> + err = devlink_health_report(reporter, msg, priv_ctx);
> + if (err) {
> + struct ice_pf *pf = devlink_health_reporter_priv(reporter);
> +
> + dev_err(ice_pf_to_dev(pf),
> + "failed to report %s via devlink health, err %d\n",
> + msg, err);
My knee-jerk reaction is - why not put it in devlink_health_report()?
Also, I'd rate limit the message.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter
2024-07-14 14:23 ` Jakub Kicinski
@ 2024-07-22 9:23 ` Mateusz Polchlopek
2024-07-30 9:33 ` Mateusz Polchlopek
0 siblings, 1 reply; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-22 9:23 UTC (permalink / raw)
To: Jakub Kicinski
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Igor Bagnucki, Wojciech Drewek
On 7/14/2024 4:23 PM, Jakub Kicinski wrote:
> On Fri, 12 Jul 2024 05:32:48 -0400 Mateusz Polchlopek wrote:
>> + err = devlink_health_report(reporter, msg, priv_ctx);
>> + if (err) {
>> + struct ice_pf *pf = devlink_health_reporter_priv(reporter);
>> +
>> + dev_err(ice_pf_to_dev(pf),
>> + "failed to report %s via devlink health, err %d\n",
>> + msg, err);
>
> My knee-jerk reaction is - why not put it in devlink_health_report()?
> Also, I'd rate limit the message.
Hmmm... That's good point. I will talk to the author about that but
seems to be good point Jakub.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter
2024-07-22 9:23 ` Mateusz Polchlopek
@ 2024-07-30 9:33 ` Mateusz Polchlopek
0 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-30 9:33 UTC (permalink / raw)
To: Jakub Kicinski
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Igor Bagnucki, Wojciech Drewek
On 7/22/2024 11:23 AM, Mateusz Polchlopek wrote:
>
>
> On 7/14/2024 4:23 PM, Jakub Kicinski wrote:
>> On Fri, 12 Jul 2024 05:32:48 -0400 Mateusz Polchlopek wrote:
>>> + err = devlink_health_report(reporter, msg, priv_ctx);
>>> + if (err) {
>>> + struct ice_pf *pf = devlink_health_reporter_priv(reporter);
>>> +
>>> + dev_err(ice_pf_to_dev(pf),
>>> + "failed to report %s via devlink health, err %d\n",
>>> + msg, err);
>>
>> My knee-jerk reaction is - why not put it in devlink_health_report()?
>> Also, I'd rate limit the message.
>
> Hmmm... That's good point. I will talk to the author about that but
> seems to be good point Jakub.
>
I took a look into the code and it seems that we do not do auto
recovering, so also ignoring return value which always will be 0.
I will remove this error checking in v3 version.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Intel-wired-lan] [PATCH iwl-next v2 4/6] ice: print ethtool stats as part of Tx hang devlink health reporter
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
` (2 preceding siblings ...)
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 3/6] ice: add Tx hang devlink health reporter Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 5/6] ice: Add MDD logging via devlink health Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang Mateusz Polchlopek
5 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Przemek Kitszel, Igor Bagnucki,
Wojciech Drewek, Simon Horman, Mateusz Polchlopek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Print the ethtool stats as part of Tx hang devlink health dump.
Move the declarations of ethtool functions that devlink health uses out
to a new file: ice_ethtool_common.h
To utilize our existing ethtool code in this context, convert it to
non-static.
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
.../intel/ice/devlink/devlink_health.c | 32 +++++++++++++++++++
drivers/net/ethernet/intel/ice/ice_ethtool.c | 10 +++---
drivers/net/ethernet/intel/ice/ice_ethtool.h | 2 ++
.../ethernet/intel/ice/ice_ethtool_common.h | 19 +++++++++++
4 files changed, 58 insertions(+), 5 deletions(-)
create mode 100644 drivers/net/ethernet/intel/ice/ice_ethtool_common.h
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
index 41f203cbdf6a..2fbf6f08d0f6 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
@@ -3,6 +3,7 @@
#include "devlink_health.h"
#include "ice.h"
+#include "ice_ethtool_common.h"
#define ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, obj, name) \
devlink_fmsg_put(fmsg, #name, (obj)->name)
@@ -32,6 +33,36 @@ static void ice_devlink_health_report(struct devlink_health_reporter *reporter,
}
}
+static void ice_dump_ethtool_stats_to_fmsg(struct devlink_fmsg *fmsg,
+ struct net_device *netdev)
+{
+ const u32 string_set = ETH_SS_STATS;
+ u64 *stats;
+ u8 *names;
+ int scnt;
+
+ scnt = ice_get_sset_count(netdev, string_set);
+ devlink_fmsg_put(fmsg, "stats-cnt", (u32)scnt);
+ if (scnt <= 0)
+ return;
+
+ names = kcalloc(scnt, ETH_GSTRING_LEN, GFP_KERNEL);
+ stats = kcalloc(scnt, sizeof(*stats), GFP_KERNEL);
+ if (!names || !stats)
+ goto out;
+
+ ice_get_strings(netdev, string_set, names);
+ ice_get_ethtool_stats(netdev, NULL, stats);
+
+ devlink_fmsg_obj_nest_start(fmsg);
+ for (int i = 0; i < scnt; ++i)
+ devlink_fmsg_put(fmsg, &names[i * ETH_GSTRING_LEN], stats[i]);
+ devlink_fmsg_obj_nest_end(fmsg);
+out:
+ kfree(names);
+ kfree(stats);
+}
+
/**
* ice_fmsg_put_ptr - put hex value of pointer into fmsg
*
@@ -76,6 +107,7 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)(long)event->tx_ring->dma);
devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
(event->tx_ring->count * sizeof(struct ice_tx_desc)));
+ ice_dump_ethtool_stats_to_fmsg(fmsg, event->tx_ring->vsi->netdev);
devlink_fmsg_obj_nest_end(fmsg);
return 0;
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 39d2652c3ee1..bc431e692b28 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -1519,7 +1519,7 @@ __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
}
}
-static void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
+void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
{
struct ice_netdev_priv *np = netdev_priv(netdev);
@@ -1897,7 +1897,7 @@ static int ice_set_priv_flags(struct net_device *netdev, u32 flags)
return ret;
}
-static int ice_get_sset_count(struct net_device *netdev, int sset)
+int ice_get_sset_count(struct net_device *netdev, int sset)
{
switch (sset) {
case ETH_SS_STATS:
@@ -2000,9 +2000,9 @@ __ice_get_ethtool_stats(struct net_device *netdev,
}
}
-static void
-ice_get_ethtool_stats(struct net_device *netdev,
- struct ethtool_stats __always_unused *stats, u64 *data)
+void ice_get_ethtool_stats(struct net_device *netdev,
+ struct ethtool_stats __always_unused *stats,
+ u64 *data)
{
struct ice_netdev_priv *np = netdev_priv(netdev);
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.h b/drivers/net/ethernet/intel/ice/ice_ethtool.h
index 9acccae38625..fd021d2813f8 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.h
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.h
@@ -4,6 +4,8 @@
#ifndef _ICE_ETHTOOL_H_
#define _ICE_ETHTOOL_H_
+#include "ice_ethtool_common.h"
+
struct ice_phy_type_to_ethtool {
u64 aq_link_speed;
u8 link_mode;
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_common.h b/drivers/net/ethernet/intel/ice/ice_ethtool_common.h
new file mode 100644
index 000000000000..0c772056f006
--- /dev/null
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool_common.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2024, Intel Corporation. */
+
+#ifndef _ICE_ETHTOOL_COMMON_H_
+#define _ICE_ETHTOOL_COMMON_H_
+
+/**
+ * DOC: ice_ethtool_common.h
+ *
+ * This header is for ethtool related code that is reused in other places.
+ */
+
+void ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data);
+int ice_get_sset_count(struct net_device *netdev, int sset);
+void ice_get_ethtool_stats(struct net_device *netdev,
+ struct ethtool_stats __always_unused *stats,
+ u64 *data);
+
+#endif /* _ICE_ETHTOOL_COMMON_H_ */
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Intel-wired-lan] [PATCH iwl-next v2 5/6] ice: Add MDD logging via devlink health
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
` (3 preceding siblings ...)
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 4/6] ice: print ethtool stats as part of " Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang Mateusz Polchlopek
5 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Ben Shelton, Przemek Kitszel, Igor Bagnucki,
Wojciech Drewek, Simon Horman, Mateusz Polchlopek
From: Ben Shelton <benjamin.h.shelton@intel.com>
Add a devlink health reporter for MDD events. The 'dump' handler will
return the information captured in each call to ice_handle_mdd_event().
A device reset (CORER/PFR) will put the reporter back in healthy state.
Signed-off-by: Ben Shelton <benjamin.h.shelton@intel.com>
Co-developed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Igor Bagnucki <igor.bagnucki@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
.../intel/ice/devlink/devlink_health.c | 77 +++++++++++++++++++
.../intel/ice/devlink/devlink_health.h | 11 +++
drivers/net/ethernet/intel/ice/ice_main.c | 6 ++
3 files changed, 94 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
index 2fbf6f08d0f6..f9edfabc9be8 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
@@ -33,6 +33,79 @@ static void ice_devlink_health_report(struct devlink_health_reporter *reporter,
}
}
+struct ice_mdd_event {
+ enum ice_mdd_src src;
+ u16 vf_num;
+ u16 queue;
+ u8 pf_num;
+ u8 event;
+};
+
+static const char *ice_mdd_src_to_str(enum ice_mdd_src src)
+{
+ switch (src) {
+ case ICE_MDD_SRC_TX_PQM:
+ return "tx_pqm";
+ case ICE_MDD_SRC_TX_TCLAN:
+ return "tx_tclan";
+ case ICE_MDD_SRC_TX_TDPU:
+ return "tx_tdpu";
+ case ICE_MDD_SRC_RX:
+ return "rx";
+ default:
+ return "invalid";
+ }
+}
+
+static int
+ice_mdd_reporter_dump(struct devlink_health_reporter *reporter,
+ struct devlink_fmsg *fmsg, void *priv_ctx,
+ struct netlink_ext_ack *extack)
+{
+ struct ice_mdd_event *mdd_event = priv_ctx;
+ const char *src;
+
+ if (!mdd_event)
+ return 0;
+
+ src = ice_mdd_src_to_str(mdd_event->src);
+
+ devlink_fmsg_obj_nest_start(fmsg);
+ devlink_fmsg_put(fmsg, "src", src);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, mdd_event, pf_num);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, mdd_event, vf_num);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, mdd_event, event);
+ ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, mdd_event, queue);
+ devlink_fmsg_obj_nest_end(fmsg);
+
+ return 0;
+}
+
+/**
+ * ice_devlink_report_mdd_event - Report an MDD event through devlink health
+ * @pf: the PF device structure
+ * @src: the HW block that was the source of this MDD event
+ * @pf_num: the pf_num on which the MDD event occurred
+ * @vf_num: the vf_num on which the MDD event occurred
+ * @event: the event type of the MDD event
+ * @queue: the queue on which the MDD event occurred
+ *
+ * Report an MDD event that has occurred on this PF.
+ */
+void ice_devlink_report_mdd_event(struct ice_pf *pf, enum ice_mdd_src src,
+ u8 pf_num, u16 vf_num, u8 event, u16 queue)
+{
+ struct ice_mdd_event ev = {
+ .src = src,
+ .pf_num = pf_num,
+ .vf_num = vf_num,
+ .event = event,
+ .queue = queue,
+ };
+
+ ice_devlink_health_report(pf->health_reporters.mdd, "MDD event", &ev);
+}
+
static void ice_dump_ethtool_stats_to_fmsg(struct devlink_fmsg *fmsg,
struct net_device *netdev)
{
@@ -154,6 +227,7 @@ ice_init_devlink_rep(struct ice_pf *pf,
.dump = ice_ ## _name ## _reporter_dump, \
}
+ICE_DEFINE_HEALTH_REPORTER_OPS(mdd);
ICE_DEFINE_HEALTH_REPORTER_OPS(tx_hang);
/**
@@ -166,6 +240,7 @@ void ice_health_init(struct ice_pf *pf)
{
struct ice_health *reps = &pf->health_reporters;
+ reps->mdd = ice_init_devlink_rep(pf, &ice_mdd_reporter_ops);
reps->tx_hang = ice_init_devlink_rep(pf, &ice_tx_hang_reporter_ops);
}
@@ -187,6 +262,7 @@ static void ice_deinit_devl_reporter(struct devlink_health_reporter *reporter)
*/
void ice_health_deinit(struct ice_pf *pf)
{
+ ice_deinit_devl_reporter(pf->health_reporters.mdd);
ice_deinit_devl_reporter(pf->health_reporters.tx_hang);
}
@@ -206,5 +282,6 @@ void ice_health_assign_healthy_state(struct devlink_health_reporter *reporter)
*/
void ice_health_clear(struct ice_pf *pf)
{
+ ice_health_assign_healthy_state(pf->health_reporters.mdd);
ice_health_assign_healthy_state(pf->health_reporters.tx_hang);
}
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.h b/drivers/net/ethernet/intel/ice/devlink/devlink_health.h
index 984b8f9f56d4..01abd3d8f31e 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink_health.h
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.h
@@ -16,18 +16,29 @@
struct ice_pf;
struct ice_tx_ring;
+enum ice_mdd_src {
+ ICE_MDD_SRC_TX_PQM,
+ ICE_MDD_SRC_TX_TCLAN,
+ ICE_MDD_SRC_TX_TDPU,
+ ICE_MDD_SRC_RX,
+};
+
/**
* struct ice_health - stores ice devlink health reporters and accompanied data
* @tx_hang: devlink health reporter for tx_hang event
+ * @mdd: devlink health reporter for MDD detection event
*/
struct ice_health {
struct devlink_health_reporter *tx_hang;
+ struct devlink_health_reporter *mdd;
};
void ice_health_init(struct ice_pf *pf);
void ice_health_deinit(struct ice_pf *pf);
void ice_health_clear(struct ice_pf *pf);
+void ice_devlink_report_mdd_event(struct ice_pf *pf, enum ice_mdd_src src,
+ u8 pf_num, u16 vf_num, u8 event, u16 queue);
void ice_report_tx_hang(struct ice_pf *pf, struct ice_tx_ring *tx_ring,
u16 vsi_num, u32 head, u32 intr);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 1fae7d34f2e4..a7c186fb902b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1819,6 +1819,8 @@ static void ice_handle_mdd_event(struct ice_pf *pf)
if (netif_msg_tx_err(pf))
dev_info(dev, "Malicious Driver Detection event %d on TX queue %d PF# %d VF# %d\n",
event, queue, pf_num, vf_num);
+ ice_devlink_report_mdd_event(pf, ICE_MDD_SRC_TX_PQM, pf_num,
+ vf_num, event, queue);
wr32(hw, GL_MDET_TX_PQM, 0xffffffff);
}
@@ -1832,6 +1834,8 @@ static void ice_handle_mdd_event(struct ice_pf *pf)
if (netif_msg_tx_err(pf))
dev_info(dev, "Malicious Driver Detection event %d on TX queue %d PF# %d VF# %d\n",
event, queue, pf_num, vf_num);
+ ice_devlink_report_mdd_event(pf, ICE_MDD_SRC_TX_TCLAN, pf_num,
+ vf_num, event, queue);
wr32(hw, GL_MDET_TX_TCLAN_BY_MAC(hw), U32_MAX);
}
@@ -1845,6 +1849,8 @@ static void ice_handle_mdd_event(struct ice_pf *pf)
if (netif_msg_rx_err(pf))
dev_info(dev, "Malicious Driver Detection event %d on RX queue %d PF# %d VF# %d\n",
event, queue, pf_num, vf_num);
+ ice_devlink_report_mdd_event(pf, ICE_MDD_SRC_RX, pf_num,
+ vf_num, event, queue);
wr32(hw, GL_MDET_RX, 0xffffffff);
}
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang
2024-07-12 9:32 [Intel-wired-lan] [PATCH iwl-next v2 0/6] Add support for devlink health events Mateusz Polchlopek
` (4 preceding siblings ...)
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 5/6] ice: Add MDD logging via devlink health Mateusz Polchlopek
@ 2024-07-12 9:32 ` Mateusz Polchlopek
2024-07-14 14:30 ` Jakub Kicinski
5 siblings, 1 reply; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-12 9:32 UTC (permalink / raw)
To: intel-wired-lan
Cc: apw, joe, dwaipayanray1, lukas.bulwahn, akpm, willemb, edumazet,
linux-kernel, netdev, Przemek Kitszel, Wojciech Drewek,
Simon Horman, Mateusz Polchlopek
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Extend Tx hang devlink health reporter dump by skb content.
This commit includes much code copy-pasted from:
- net/core/skbuff.c (function skb_dump() - modified to dump into buffer)
- lib/hexdump.c (function print_hex_dump())
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
---
.../intel/ice/devlink/devlink_health.c | 197 ++++++++++++++++++
1 file changed, 197 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
index f9edfabc9be8..1a9b19a7e7e1 100644
--- a/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
+++ b/drivers/net/ethernet/intel/ice/devlink/devlink_health.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2024, Intel Corporation. */
+#include <net/genetlink.h>
#include "devlink_health.h"
#include "ice.h"
#include "ice_ethtool_common.h"
@@ -136,6 +137,188 @@ static void ice_dump_ethtool_stats_to_fmsg(struct devlink_fmsg *fmsg,
kfree(stats);
}
+/**
+ * ice_emit_to_buf - print to @size sized buffer
+ *
+ * @buf: buffer to print into
+ * @at: current pos to write in @buf
+ * @size: total space in @buf (incl. prior to @at)
+ * @fmt: format of the message to print
+ *
+ * Return: position in the @buf for next write, @size at most, to ease out
+ * error handling.
+ */
+static __printf(4, 5)
+int ice_emit_to_buf(char *buf, int size, int at, const char *fmt, ...)
+{
+ va_list args;
+ int len;
+
+ va_start(args, fmt);
+ len = vscnprintf(buf + at, size - at, fmt, args);
+ va_end(args);
+
+ return min(at + len, size);
+}
+
+/**
+ * ice_emit_hex_to_buf - copy of print_hex_dump() from lib/hexdump.c modified to
+ * dump into buffer
+ *
+ * @out: buffer to print to
+ * @out_size: total size of @out
+ * @out_pos: position in @out to write at
+ * @prefix: string to prefix each line with;
+ * caller supplies trailing spaces for alignment if desired
+ * @buf: data blob to dump
+ * @buf_len: number of bytes in the @buf
+ *
+ * Return: position in the buf for next write, buf_len at most, to ease out
+ * error handling.
+ */
+static int ice_emit_hex_to_buf(char *out, int out_size, int out_pos,
+ const char *prefix, const void *buf,
+ size_t buf_len)
+{
+ unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+ const int rowsize = 16, groupsize = 1;
+ int i, linelen, remaining = buf_len;
+ const u8 *ptr = buf;
+
+ for (i = 0; i < buf_len; i += rowsize) {
+ linelen = min(remaining, rowsize);
+ remaining -= rowsize;
+
+ hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
+ linebuf, sizeof(linebuf), false);
+ out_pos = ice_emit_to_buf(out, out_size, out_pos,
+ "%s%.8x: %s\n", prefix, i, linebuf);
+
+ if (out_pos == out_size)
+ break;
+ }
+
+ return out_pos;
+}
+
+/**
+ * ice_skb_dump_buf - Dump skb information and contents.
+ *
+ * copy of skb_dump() from net/core/skbuff.c, modified to dump into buffer
+ *
+ * @skb: skb to dump
+ * @buf: buffer to dump into
+ * @buf_size: size of @buf
+ * @buf_pos: current position to write in @buf
+ *
+ * Return: position in the buf for next write.
+ */
+static int ice_skb_dump_buf(char *buf, int buf_size, int buf_pos,
+ const struct sk_buff *skb)
+{
+ struct skb_shared_info *sh = skb_shinfo(skb);
+ struct net_device *dev = skb->dev;
+ const bool toplvl = !buf_pos;
+ struct sock *sk = skb->sk;
+ struct sk_buff *list_skb;
+ bool has_mac, has_trans;
+ int headroom, tailroom;
+ int i, len, seg_len;
+
+ len = skb->len;
+
+ headroom = skb_headroom(skb);
+ tailroom = skb_tailroom(skb);
+
+ has_mac = skb_mac_header_was_set(skb);
+ has_trans = skb_transport_header_was_set(skb);
+
+ buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
+ "skb len=%u headroom=%u headlen=%u tailroom=%u\n"
+ "mac=(%d,%d) net=(%d,%d) trans=%d\n"
+ "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
+ "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
+ "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
+ skb->len, headroom, skb_headlen(skb), tailroom,
+ has_mac ? skb->mac_header : -1,
+ has_mac ? skb_mac_header_len(skb) : -1,
+ skb->network_header,
+ has_trans ? skb_network_header_len(skb) : -1,
+ has_trans ? skb->transport_header : -1,
+ sh->tx_flags, sh->nr_frags,
+ sh->gso_size, sh->gso_type, sh->gso_segs,
+ skb->csum, skb->ip_summed, skb->csum_complete_sw,
+ skb->csum_valid, skb->csum_level,
+ skb->hash, skb->sw_hash, skb->l4_hash,
+ ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
+
+ if (dev)
+ buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
+ "dev name=%s feat=%pNF\n", dev->name,
+ &dev->features);
+ if (sk)
+ buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
+ "sk family=%hu type=%u proto=%u\n",
+ sk->sk_family, sk->sk_type,
+ sk->sk_protocol);
+
+ if (headroom)
+ buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
+ "skb headroom: ", skb->head,
+ headroom);
+
+ seg_len = min_t(int, skb_headlen(skb), len);
+ if (seg_len)
+ buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
+ "skb linear: ", skb->data,
+ seg_len);
+ len -= seg_len;
+
+ if (tailroom)
+ buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
+ "skb tailroom: ",
+ skb_tail_pointer(skb), tailroom);
+
+ for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
+ skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+ u32 p_off, p_len, copied;
+ struct page *p;
+ u8 *vaddr;
+
+ skb_frag_foreach_page(frag, skb_frag_off(frag),
+ skb_frag_size(frag), p, p_off, p_len,
+ copied) {
+ seg_len = min_t(int, p_len, len);
+ vaddr = kmap_local_page(p);
+ buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
+ "skb frag: ",
+ vaddr + p_off, seg_len);
+ kunmap_local(vaddr);
+ len -= seg_len;
+
+ if (!len || buf_pos == buf_size)
+ break;
+ }
+ }
+
+ if (skb_has_frag_list(skb)) {
+ buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
+ "skb fraglist:\n");
+ skb_walk_frags(skb, list_skb) {
+ buf_pos = ice_skb_dump_buf(buf, buf_size, buf_pos,
+ list_skb);
+
+ if (buf_pos == buf_size)
+ break;
+ }
+ }
+
+ if (toplvl)
+ buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos, ".");
+
+ return buf_pos;
+}
+
/**
* ice_fmsg_put_ptr - put hex value of pointer into fmsg
*
@@ -167,6 +350,10 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
struct netlink_ext_ack *extack)
{
struct ice_tx_hang_event *event = priv_ctx;
+ char *skb_txt = NULL;
+ struct sk_buff *skb;
+
+ skb = event->tx_ring->tx_buf->skb;
devlink_fmsg_obj_nest_start(fmsg);
ICE_DEVLINK_FMSG_PUT_FIELD(fmsg, event, head);
@@ -178,8 +365,18 @@ static int ice_tx_hang_reporter_dump(struct devlink_health_reporter *reporter,
devlink_fmsg_put(fmsg, "irq-mapping", event->tx_ring->q_vector->name);
ice_fmsg_put_ptr(fmsg, "desc-ptr", event->tx_ring->desc);
ice_fmsg_put_ptr(fmsg, "dma-ptr", (void *)(long)event->tx_ring->dma);
+ ice_fmsg_put_ptr(fmsg, "skb-ptr", skb);
devlink_fmsg_binary_pair_put(fmsg, "desc", event->tx_ring->desc,
(event->tx_ring->count * sizeof(struct ice_tx_desc)));
+ if (skb)
+ skb_txt = kvmalloc(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+
+ if (skb_txt) {
+ ice_skb_dump_buf(skb_txt, GENLMSG_DEFAULT_SIZE, 0, skb);
+ devlink_fmsg_put(fmsg, "skb", skb_txt);
+ kvfree(skb_txt);
+ }
+
ice_dump_ethtool_stats_to_fmsg(fmsg, event->tx_ring->vsi->netdev);
devlink_fmsg_obj_nest_end(fmsg);
--
2.38.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang
2024-07-12 9:32 ` [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang Mateusz Polchlopek
@ 2024-07-14 14:30 ` Jakub Kicinski
2024-07-22 9:23 ` Mateusz Polchlopek
2024-07-30 9:35 ` Mateusz Polchlopek
0 siblings, 2 replies; 13+ messages in thread
From: Jakub Kicinski @ 2024-07-14 14:30 UTC (permalink / raw)
To: Mateusz Polchlopek
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Wojciech Drewek, Simon Horman
On Fri, 12 Jul 2024 05:32:51 -0400 Mateusz Polchlopek wrote:
> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
> + "skb len=%u headroom=%u headlen=%u tailroom=%u\n"
> + "mac=(%d,%d) net=(%d,%d) trans=%d\n"
> + "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
> + "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
> + "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
> + skb->len, headroom, skb_headlen(skb), tailroom,
> + has_mac ? skb->mac_header : -1,
> + has_mac ? skb_mac_header_len(skb) : -1,
> + skb->network_header,
> + has_trans ? skb_network_header_len(skb) : -1,
> + has_trans ? skb->transport_header : -1,
> + sh->tx_flags, sh->nr_frags,
> + sh->gso_size, sh->gso_type, sh->gso_segs,
> + skb->csum, skb->ip_summed, skb->csum_complete_sw,
> + skb->csum_valid, skb->csum_level,
> + skb->hash, skb->sw_hash, skb->l4_hash,
> + ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
Make it a generic helper in devlink?
> + if (dev)
> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
> + "dev name=%s feat=%pNF\n", dev->name,
> + &dev->features);
> + if (sk)
> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
> + "sk family=%hu type=%u proto=%u\n",
> + sk->sk_family, sk->sk_type,
> + sk->sk_protocol);
> +
> + if (headroom)
> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
> + "skb headroom: ", skb->head,
> + headroom);
> +
> + seg_len = min_t(int, skb_headlen(skb), len);
> + if (seg_len)
> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
> + "skb linear: ", skb->data,
> + seg_len);
> + len -= seg_len;
> +
> + if (tailroom)
> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
> + "skb tailroom: ",
> + skb_tail_pointer(skb), tailroom);
The printing on tailroom, headroom and frag data seems a bit much.
I guess you're only printing the head SKB so it may be fine. But
I don't think it's useful. The device will probably only care about
the contents of the headers, for other parts only the metadata matters.
No strong preference tho.
> + for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> + u32 p_off, p_len, copied;
> + struct page *p;
> + u8 *vaddr;
> +
> + skb_frag_foreach_page(frag, skb_frag_off(frag),
> + skb_frag_size(frag), p, p_off, p_len,
> + copied) {
> + seg_len = min_t(int, p_len, len);
> + vaddr = kmap_local_page(p);
> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
> + "skb frag: ",
> + vaddr + p_off, seg_len);
> + kunmap_local(vaddr);
> + len -= seg_len;
> +
> + if (!len || buf_pos == buf_size)
> + break;
> + }
> + }
> +
> + if (skb_has_frag_list(skb)) {
> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
> + "skb fraglist:\n");
> + skb_walk_frags(skb, list_skb) {
> + buf_pos = ice_skb_dump_buf(buf, buf_size, buf_pos,
> + list_skb);
> +
> + if (buf_pos == buf_size)
> + break;
> + }
> + }
You support transmitting skbs with fraglist? 🤨️
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang
2024-07-14 14:30 ` Jakub Kicinski
@ 2024-07-22 9:23 ` Mateusz Polchlopek
2024-07-30 9:35 ` Mateusz Polchlopek
1 sibling, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-22 9:23 UTC (permalink / raw)
To: Jakub Kicinski
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Wojciech Drewek, Simon Horman
On 7/14/2024 4:30 PM, Jakub Kicinski wrote:
> On Fri, 12 Jul 2024 05:32:51 -0400 Mateusz Polchlopek wrote:
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "skb len=%u headroom=%u headlen=%u tailroom=%u\n"
>> + "mac=(%d,%d) net=(%d,%d) trans=%d\n"
>> + "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
>> + "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
>> + "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
>> + skb->len, headroom, skb_headlen(skb), tailroom,
>> + has_mac ? skb->mac_header : -1,
>> + has_mac ? skb_mac_header_len(skb) : -1,
>> + skb->network_header,
>> + has_trans ? skb_network_header_len(skb) : -1,
>> + has_trans ? skb->transport_header : -1,
>> + sh->tx_flags, sh->nr_frags,
>> + sh->gso_size, sh->gso_type, sh->gso_segs,
>> + skb->csum, skb->ip_summed, skb->csum_complete_sw,
>> + skb->csum_valid, skb->csum_level,
>> + skb->hash, skb->sw_hash, skb->l4_hash,
>> + ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
>
> Make it a generic helper in devlink?
Makes sense for me but need to consult with an author
>
>> + if (dev)
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "dev name=%s feat=%pNF\n", dev->name,
>> + &dev->features);
>> + if (sk)
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "sk family=%hu type=%u proto=%u\n",
>> + sk->sk_family, sk->sk_type,
>> + sk->sk_protocol);
>> +
>> + if (headroom)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb headroom: ", skb->head,
>> + headroom);
>> +
>> + seg_len = min_t(int, skb_headlen(skb), len);
>> + if (seg_len)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb linear: ", skb->data,
>> + seg_len);
>> + len -= seg_len;
>> +
>> + if (tailroom)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb tailroom: ",
>> + skb_tail_pointer(skb), tailroom);
>
> The printing on tailroom, headroom and frag data seems a bit much.
> I guess you're only printing the head SKB so it may be fine. But
> I don't think it's useful. The device will probably only care about
> the contents of the headers, for other parts only the metadata matters.
> No strong preference tho.
>
>> + for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
>> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>> + u32 p_off, p_len, copied;
>> + struct page *p;
>> + u8 *vaddr;
>> +
>> + skb_frag_foreach_page(frag, skb_frag_off(frag),
>> + skb_frag_size(frag), p, p_off, p_len,
>> + copied) {
>> + seg_len = min_t(int, p_len, len);
>> + vaddr = kmap_local_page(p);
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb frag: ",
>> + vaddr + p_off, seg_len);
>> + kunmap_local(vaddr);
>> + len -= seg_len;
>> +
>> + if (!len || buf_pos == buf_size)
>> + break;
>> + }
>> + }
>> +
>> + if (skb_has_frag_list(skb)) {
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "skb fraglist:\n");
>> + skb_walk_frags(skb, list_skb) {
>> + buf_pos = ice_skb_dump_buf(buf, buf_size, buf_pos,
>> + list_skb);
>> +
>> + if (buf_pos == buf_size)
>> + break;
>> + }
>> + }
>
> You support transmitting skbs with fraglist? 🤨️
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v2 6/6] ice: devlink health: dump also skb on Tx hang
2024-07-14 14:30 ` Jakub Kicinski
2024-07-22 9:23 ` Mateusz Polchlopek
@ 2024-07-30 9:35 ` Mateusz Polchlopek
1 sibling, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-07-30 9:35 UTC (permalink / raw)
To: Jakub Kicinski
Cc: intel-wired-lan, apw, joe, dwaipayanray1, lukas.bulwahn, akpm,
willemb, edumazet, linux-kernel, netdev, Przemek Kitszel,
Wojciech Drewek, Simon Horman
On 7/14/2024 4:30 PM, Jakub Kicinski wrote:
> On Fri, 12 Jul 2024 05:32:51 -0400 Mateusz Polchlopek wrote:
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "skb len=%u headroom=%u headlen=%u tailroom=%u\n"
>> + "mac=(%d,%d) net=(%d,%d) trans=%d\n"
>> + "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
>> + "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
>> + "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
>> + skb->len, headroom, skb_headlen(skb), tailroom,
>> + has_mac ? skb->mac_header : -1,
>> + has_mac ? skb_mac_header_len(skb) : -1,
>> + skb->network_header,
>> + has_trans ? skb_network_header_len(skb) : -1,
>> + has_trans ? skb->transport_header : -1,
>> + sh->tx_flags, sh->nr_frags,
>> + sh->gso_size, sh->gso_type, sh->gso_segs,
>> + skb->csum, skb->ip_summed, skb->csum_complete_sw,
>> + skb->csum_valid, skb->csum_level,
>> + skb->hash, skb->sw_hash, skb->l4_hash,
>> + ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
>
> Make it a generic helper in devlink?
>
>> + if (dev)
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "dev name=%s feat=%pNF\n", dev->name,
>> + &dev->features);
>> + if (sk)
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "sk family=%hu type=%u proto=%u\n",
>> + sk->sk_family, sk->sk_type,
>> + sk->sk_protocol);
>> +
>> + if (headroom)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb headroom: ", skb->head,
>> + headroom);
>> +
>> + seg_len = min_t(int, skb_headlen(skb), len);
>> + if (seg_len)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb linear: ", skb->data,
>> + seg_len);
>> + len -= seg_len;
>> +
>> + if (tailroom)
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb tailroom: ",
>> + skb_tail_pointer(skb), tailroom);
>
> The printing on tailroom, headroom and frag data seems a bit much.
> I guess you're only printing the head SKB so it may be fine. But
> I don't think it's useful. The device will probably only care about
> the contents of the headers, for other parts only the metadata matters.
> No strong preference tho.
> >> + for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
>> + skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>> + u32 p_off, p_len, copied;
>> + struct page *p;
>> + u8 *vaddr;
>> +
>> + skb_frag_foreach_page(frag, skb_frag_off(frag),
>> + skb_frag_size(frag), p, p_off, p_len,
>> + copied) {
>> + seg_len = min_t(int, p_len, len);
>> + vaddr = kmap_local_page(p);
>> + buf_pos = ice_emit_hex_to_buf(buf, buf_size, buf_pos,
>> + "skb frag: ",
>> + vaddr + p_off, seg_len);
>> + kunmap_local(vaddr);
>> + len -= seg_len;
>> +
>> + if (!len || buf_pos == buf_size)
>> + break;
>> + }
>> + }
>> +
>> + if (skb_has_frag_list(skb)) {
>> + buf_pos = ice_emit_to_buf(buf, buf_size, buf_pos,
>> + "skb fraglist:\n");
>> + skb_walk_frags(skb, list_skb) {
>> + buf_pos = ice_skb_dump_buf(buf, buf_size, buf_pos,
>> + list_skb);
>> +
>> + if (buf_pos == buf_size)
>> + break;
>> + }
>> + }
>
> You support transmitting skbs with fraglist? 🤨️
This will be removed in the next version, we will log only the number of
frags
^ permalink raw reply [flat|nested] 13+ messages in thread