[net] devlink: Add method for time-stamp on reporter's dump

[net] devlink: Add method for time-stamp on reporter's dump

[Aya Levin]

When setting the dump's time-stamp, use ktime_get_real in addition to
jiffies. This simplifies the user space implementation and bypasses
some inconsistent behavior with translating jiffies to current time.

Fixes: c8e1da0bf923 ("devlink: Add health report functionality")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 include/uapi/linux/devlink.h | 2 ++
 net/core/devlink.c           | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index ffc993256527..4dd4e4e7b19b 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -348,6 +348,8 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
 	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
 
+	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC,
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index d3dbb904bf3b..b26875c4329b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -4577,6 +4577,7 @@ struct devlink_health_reporter {
 	bool auto_recover;
 	u8 health_state;
 	u64 dump_ts;
+	struct timespec dump_real_ts;
 	u64 error_count;
 	u64 recovery_count;
 	u64 last_recovery_ts;
@@ -4749,6 +4750,7 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
 		goto dump_err;
 
 	reporter->dump_ts = jiffies;
+	reporter->dump_real_ts = ktime_to_timespec(ktime_get_real());
 
 	return 0;
 
@@ -4911,6 +4913,10 @@ devlink_nl_health_reporter_fill(struct sk_buff *msg,
 			      jiffies_to_msecs(reporter->dump_ts),
 			      DEVLINK_ATTR_PAD))
 		goto reporter_nest_cancel;
+	if (reporter->dump_fmsg &&
+	    nla_put(msg, DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC,
+		    sizeof(reporter->dump_real_ts), &reporter->dump_real_ts))
+		goto reporter_nest_cancel;
 
 	nla_nest_end(msg, reporter_attr);
 	genlmsg_end(msg, hdr);
-- 
2.14.1

[iproute2, master 0/2] Fix reporter's dump's time-stamp

[Aya Levin]

This patch set handles the reporter's dump time-stamp display. First
patch refactors the current implementation of helper function which
displays the reporter's dump time-stamp and add the actual print to
the function's body.

The second patch introduces a new attribute which is the time-stamp in
timespec (current time) instead of jiffies. When the new attribute is
present try and translate the time-stamp according to 'current time'.

Aya Levin (2):
  devlink: Print health reporter's dump time-stamp in a helper function
  devlink: Add a new time-stamp format for health reporter's dump

 devlink/devlink.c            | 44 ++++++++++++++++++++++++++++----------------
 include/uapi/linux/devlink.h |  2 ++
 2 files changed, 30 insertions(+), 16 deletions(-)

-- 
2.14.1

[iproute2, master 1/2] devlink: Print health reporter's dump time-stamp in a helper function

[Aya Levin]

Add pr_out_dump_reporter prefix to the helper function's name and
encapsulate the print in it.

Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 devlink/devlink.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 055eca5d4662..f1d9de8e151d 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6169,8 +6169,11 @@ static const char *health_state_name(uint8_t state)
 	}
 }
 
-static void format_logtime(uint64_t time_ms, char *ts_date, char *ts_time)
+static void pr_out_dump_reporter_format_logtime(struct dl *dl, const struct nlattr *attr)
 {
+	char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	uint64_t time_ms = mnl_attr_get_u64(attr);
 	struct sysinfo s_info;
 	struct tm *info;
 	time_t now, sec;
@@ -6188,16 +6191,16 @@ static void format_logtime(uint64_t time_ms, char *ts_date, char *ts_time)
 	sec = now - s_info.uptime + time_ms / 1000;
 	info = localtime(&sec);
 out:
-	strftime(ts_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", info);
-	strftime(ts_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", info);
+	strftime(dump_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", info);
+	strftime(dump_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", info);
+	pr_out_str(dl, "last_dump_date", dump_date);
+	pr_out_str(dl, "last_dump_time", dump_time);
 }
 
 static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
 	enum devlink_health_reporter_state state;
-	const struct nlattr *attr;
-	uint64_t time_ms;
 	int err;
 
 	err = mnl_attr_parse_nested(tb_health[DEVLINK_ATTR_HEALTH_REPORTER],
@@ -6225,17 +6228,8 @@ static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT]));
 	pr_out_u64(dl, "recover",
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT]));
-	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]) {
-		char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
-		char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
-
-		attr = tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS];
-		time_ms = mnl_attr_get_u64(attr);
-		format_logtime(time_ms, dump_date, dump_time);
-
-		pr_out_str(dl, "last_dump_date", dump_date);
-		pr_out_str(dl, "last_dump_time", dump_time);
-	}
+	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
+		pr_out_dump_reporter_format_logtime(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]);
 	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
 		pr_out_u64(dl, "grace_period",
 			   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD]));
-- 
2.14.1

Re: [iproute2, master 1/2] devlink: Print health reporter's dump time-stamp in a helper function

[Stephen Hemminger]

On Thu, 22 Aug 2019 14:05:41 +0300
Aya Levin <ayal@mellanox.com> wrote:

> Add pr_out_dump_reporter prefix to the helper function's name and
> encapsulate the print in it.
> 
> Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
> Signed-off-by: Aya Levin <ayal@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>


Looks fine, but devlink needs to be converted from doing JSON
printing its own way and use common iproute2 libraries.

Re: [iproute2, master 1/2] devlink: Print health reporter's dump time-stamp in a helper function

[Aya Levin]

On 8/30/2019 2:25 AM, Stephen Hemminger wrote:
> On Thu, 22 Aug 2019 14:05:41 +0300
> Aya Levin <ayal@mellanox.com> wrote:
> 
>> Add pr_out_dump_reporter prefix to the helper function's name and
>> encapsulate the print in it.
>>
>> Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
>> Signed-off-by: Aya Levin <ayal@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
> 
> 
> Looks fine, but devlink needs to be converted from doing JSON
> printing its own way and use common iproute2 libraries.
Sorry for the late response.
You are correct, it is in our plans to complete a full transition to 
common iproute2 helpers in the following weeks.
I got your point, and will not submit more patches adding new print 
functions using the current API and will wait for submission after the 
transition to the common API.
This patch will be re-submitted after the transition.
> 

[iproute2, master 2/2] devlink: Add a new time-stamp format for health reporter's dump

[Aya Levin]

Introduce a new attribute representing a new time-stamp format: current
time instead of jiffies. If the new attribute was received, translate
the time-stamp accordingly.

Fixes: 2f1242efe9d0 ("devlink: Add devlink health show command")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 devlink/devlink.c            | 20 +++++++++++++++++++-
 include/uapi/linux/devlink.h |  2 ++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index f1d9de8e151d..623d1b52c4ca 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6197,6 +6197,22 @@ out:
 	pr_out_str(dl, "last_dump_time", dump_time);
 }
 
+static void pr_out_dump_report_timestamp(struct dl *dl, const struct nlattr *attr)
+{
+	char dump_date[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	char dump_time[HEALTH_REPORTER_TIMESTAMP_FMT_LEN];
+	struct timespec *ts = mnl_attr_get_payload(attr);
+	struct tm *tm;
+
+	tm = localtime(&ts->tv_sec);
+
+	strftime(dump_date, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%Y-%m-%d", tm);
+	strftime(dump_time, HEALTH_REPORTER_TIMESTAMP_FMT_LEN, "%H:%M:%S", tm);
+
+	pr_out_str(dl, "last_dump_date", dump_date);
+	pr_out_str(dl, "last_dump_time", dump_time);
+}
+
 static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 {
 	struct nlattr *tb[DEVLINK_ATTR_MAX + 1] = {};
@@ -6228,7 +6244,9 @@ static void pr_out_health(struct dl *dl, struct nlattr **tb_health)
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_ERR_COUNT]));
 	pr_out_u64(dl, "recover",
 		   mnl_attr_get_u64(tb[DEVLINK_ATTR_HEALTH_REPORTER_RECOVER_COUNT]));
-	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
+	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC])
+		pr_out_dump_report_timestamp(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC]);
+	else if (tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS])
 		pr_out_dump_reporter_format_logtime(dl, tb[DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS]);
 	if (tb[DEVLINK_ATTR_HEALTH_REPORTER_GRACEFUL_PERIOD])
 		pr_out_u64(dl, "grace_period",
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index fc195cbd66f4..3f8532711315 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -348,6 +348,8 @@ enum devlink_attr {
 	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
 	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
 
+	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC,
+
 	/* add new attributes above here, update the policy in devlink.c */
 
 	__DEVLINK_ATTR_MAX,
-- 
2.14.1

Re: [iproute2, master 2/2] devlink: Add a new time-stamp format for health reporter's dump

[Stephen Hemminger]

On Thu, 22 Aug 2019 14:05:42 +0300
Aya Levin <ayal@mellanox.com> wrote:

> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index fc195cbd66f4..3f8532711315 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -348,6 +348,8 @@ enum devlink_attr {
>  	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
>  	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
>  
> +	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC,
> +
>  	/* add new attributes above here, update the policy in devlink.c */
>  
>  	__DEVLINK_ATTR_MAX,
> -- 

Since this is not upstream, this patch needs to go to iproute2-next.
Which means if you want the other bug fix, send it again against master.

Re: [iproute2, master 2/2] devlink: Add a new time-stamp format for health reporter's dump

[Aya Levin]

On 8/30/2019 2:27 AM, Stephen Hemminger wrote:
> On Thu, 22 Aug 2019 14:05:42 +0300
> Aya Levin <ayal@mellanox.com> wrote:
> 
>> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>> index fc195cbd66f4..3f8532711315 100644
>> --- a/include/uapi/linux/devlink.h
>> +++ b/include/uapi/linux/devlink.h
>> @@ -348,6 +348,8 @@ enum devlink_attr {
>>   	DEVLINK_ATTR_PORT_PCI_PF_NUMBER,	/* u16 */
>>   	DEVLINK_ATTR_PORT_PCI_VF_NUMBER,	/* u16 */
>>   
>> +	DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TSPEC,
>> +
>>   	/* add new attributes above here, update the policy in devlink.c */
>>   
>>   	__DEVLINK_ATTR_MAX,
>> -- 
> 
> Since this is not upstream, this patch needs to go to iproute2-next.
> Which means if you want the other bug fix, send it again against master.
Thanks,
Will do that
> 

Re: [net] devlink: Add method for time-stamp on reporter's dump

[Andrew Lunn]

On Thu, Aug 22, 2019 at 11:17:51AM +0300, Aya Levin wrote:
> When setting the dump's time-stamp, use ktime_get_real in addition to
> jiffies. This simplifies the user space implementation and bypasses
> some inconsistent behavior with translating jiffies to current time.

Hi Aya

Is this year 2038 safe? I don't know enough about this to answer the
question myself.

Thanks
	Andrew

Re: [net] devlink: Add method for time-stamp on reporter's dump

[Ido Schimmel]

On Thu, Aug 22, 2019 at 04:06:35PM +0200, Andrew Lunn wrote:
> On Thu, Aug 22, 2019 at 11:17:51AM +0300, Aya Levin wrote:
> > When setting the dump's time-stamp, use ktime_get_real in addition to
> > jiffies. This simplifies the user space implementation and bypasses
> > some inconsistent behavior with translating jiffies to current time.
> 
> Hi Aya
> 
> Is this year 2038 safe? I don't know enough about this to answer the
> question myself.

Hi Andrew,

Good point. 'struct timespec' is not considered year 2038 safe and
unfortunately I recently made the mistake of using it to communicate
timestamps to user space over netlink. :/ The code is still in net-next,
so I will fix it while I can.

Arnd, would it be acceptable to use 'struct __kernel_timespec' instead?

Thanks

Re: [net] devlink: Add method for time-stamp on reporter's dump

[Arnd Bergmann]

On Thu, Aug 22, 2019 at 7:40 PM Ido Schimmel <idosch@idosch.org> wrote:
> On Thu, Aug 22, 2019 at 04:06:35PM +0200, Andrew Lunn wrote:
> > On Thu, Aug 22, 2019 at 11:17:51AM +0300, Aya Levin wrote:
> > > When setting the dump's time-stamp, use ktime_get_real in addition to
> > > jiffies. This simplifies the user space implementation and bypasses
> > > some inconsistent behavior with translating jiffies to current time.
> >
> > Is this year 2038 safe? I don't know enough about this to answer the
> > question myself.
>
> Good point. 'struct timespec' is not considered year 2038 safe and
> unfortunately I recently made the mistake of using it to communicate
> timestamps to user space over netlink. :/ The code is still in net-next,
> so I will fix it while I can.
>
> Arnd, would it be acceptable to use 'struct __kernel_timespec' instead?

The in-kernel representation should just use 'timespec64' if you need
separate seconds and nanoseconds, you can convert that to
__kernel_timespec while copying to user space.

However, please consider two other points:

- for simplicity, the general recommendation is to use 64-bit nanoseconds
  without separate seconds for timestamps
- instead of CLOCK_REALTIME, you could use CLOCK_MONOTONIC
  timestamps that are not affected by clock_settime() or leap second jumps.

      Arnd