[PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[Roman Mashak]

When a new action is installed, firstuse field of 'tcf_t' is explicitly set
to 0. Value of zero means "new action, not yet used"; as a packet hits the
action, 'firstuse' is stamped with the current jiffies value.

tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.

Fixes: 48d8ee1694dd ("net sched actions: aggregate dumping of actions timeinfo")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/act_api.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index c24d7643548e..124bd139886c 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -75,7 +75,8 @@ static inline void tcf_tm_dump(struct tcf_t *dtm, const struct tcf_t *stm)
 {
 	dtm->install = jiffies_to_clock_t(jiffies - stm->install);
 	dtm->lastuse = jiffies_to_clock_t(jiffies - stm->lastuse);
-	dtm->firstuse = jiffies_to_clock_t(jiffies - stm->firstuse);
+	dtm->firstuse = stm->firstuse ?
+		jiffies_to_clock_t(jiffies - stm->firstuse) : 0;
 	dtm->expires = jiffies_to_clock_t(stm->expires);
 }
 
-- 
2.7.4

Re: [PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[Cong Wang]

On Sun, May 17, 2020 at 5:47 AM Roman Mashak <mrv@mojatatu.com> wrote:
>
> When a new action is installed, firstuse field of 'tcf_t' is explicitly set
> to 0. Value of zero means "new action, not yet used"; as a packet hits the
> action, 'firstuse' is stamped with the current jiffies value.
>
> tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.

Your patch makes sense to me.

Just one more thing, how about 'lastuse'? It is initialized with jiffies,
not 0, it seems we should initialize it to 0 too, as it is not yet used?

Thanks.

Re: [PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[Roman Mashak]

Cong Wang <xiyou.wangcong@gmail.com> writes:

> On Sun, May 17, 2020 at 5:47 AM Roman Mashak <mrv@mojatatu.com> wrote:
>>
>> When a new action is installed, firstuse field of 'tcf_t' is explicitly set
>> to 0. Value of zero means "new action, not yet used"; as a packet hits the
>> action, 'firstuse' is stamped with the current jiffies value.
>>
>> tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.
>
> Your patch makes sense to me.
>
> Just one more thing, how about 'lastuse'? It is initialized with jiffies,
> not 0, it seems we should initialize it to 0 too, as it is not yet used?

Yes, exactly. I was planning to send a separate patch for this.

Thanks for review, Cong.

Re: [PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[Jamal Hadi Salim]

On 2020-05-17 9:10 p.m., Roman Mashak wrote:
> Cong Wang <xiyou.wangcong@gmail.com> writes:
> 
>> On Sun, May 17, 2020 at 5:47 AM Roman Mashak <mrv@mojatatu.com> wrote:
>>>
>>> When a new action is installed, firstuse field of 'tcf_t' is explicitly set
>>> to 0. Value of zero means "new action, not yet used"; as a packet hits the
>>> action, 'firstuse' is stamped with the current jiffies value.
>>>
>>> tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.
>>
>> Your patch makes sense to me.
>>
>> Just one more thing, how about 'lastuse'? It is initialized with jiffies,
>> not 0, it seems we should initialize it to 0 too, as it is not yet used?
> 
> Yes, exactly. I was planning to send a separate patch for this.
> 
> Thanks for review, Cong.
> 

For these corner cases, firstuse using zero to indicate
"has not been used" is not ambigious.
lastuse has ambiguity because zero now has two meanings
if you check for the corner case in the kernel.
1)Zero is a legit value when dumping or
getting (example an action was just hit when you dumped).
2) zero also now means "has not been used".

My suggestion is to leave this alone in the kernel.
In user space/iproute2 check if lastused and created
are equal and declare "has not been used".

cheers,
jamal

Re: [PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[Cong Wang]

On Mon, May 18, 2020 at 5:43 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2020-05-17 9:10 p.m., Roman Mashak wrote:
> > Cong Wang <xiyou.wangcong@gmail.com> writes:
> >
> >> On Sun, May 17, 2020 at 5:47 AM Roman Mashak <mrv@mojatatu.com> wrote:
> >>>
> >>> When a new action is installed, firstuse field of 'tcf_t' is explicitly set
> >>> to 0. Value of zero means "new action, not yet used"; as a packet hits the
> >>> action, 'firstuse' is stamped with the current jiffies value.
> >>>
> >>> tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.
> >>
> >> Your patch makes sense to me.
> >>
> >> Just one more thing, how about 'lastuse'? It is initialized with jiffies,
> >> not 0, it seems we should initialize it to 0 too, as it is not yet used?
> >
> > Yes, exactly. I was planning to send a separate patch for this.
> >
> > Thanks for review, Cong.
> >
>
> For these corner cases, firstuse using zero to indicate
> "has not been used" is not ambigious.
> lastuse has ambiguity because zero now has two meanings
> if you check for the corner case in the kernel.
> 1)Zero is a legit value when dumping or
> getting (example an action was just hit when you dumped).
> 2) zero also now means "has not been used".

Well, technically firstuse could be a legit 0 too, when the
action was just hit for the first time right when dumping.
So the ambiguity is same for both.

>
> My suggestion is to leave this alone in the kernel.
> In user space/iproute2 check if lastused and created
> are equal and declare "has not been used".

Sounds a good idea to me.

Thanks.

Re: [PATCH net 1/1] net sched: fix reporting the first-time use timestamp

[David Miller]

From: Roman Mashak <mrv@mojatatu.com>
Date: Sun, 17 May 2020 08:46:31 -0400

> When a new action is installed, firstuse field of 'tcf_t' is explicitly set
> to 0. Value of zero means "new action, not yet used"; as a packet hits the
> action, 'firstuse' is stamped with the current jiffies value.
> 
> tcf_tm_dump() should return 0 for firstuse if action has not yet been hit.
> 
> Fixes: 48d8ee1694dd ("net sched actions: aggregate dumping of actions timeinfo")
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Roman Mashak <mrv@mojatatu.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Applied and queued up for -stable, thanks.