test

test

[Simon Kirby]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

Hello!

I was noticing a significant amount of what seems/seemed to be
destination lists with multiple entries with the lblcr LVS algorithm. 
While tracking it down, I think I stumbled over a mistake.  In
ip_vs_lblcr_full_check(), it appears the time check logic is reversed:

        for (i=0, j=tbl->rover; i<IP_VS_LBLCR_TAB_SIZE; i++) {
                j = (j + 1) & IP_VS_LBLCR_TAB_MASK;

                write_lock(&svc->sched_lock);
                list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
                        if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
                                       now))
                                continue;
                        
                        ip_vs_lblcr_free(en);
                        atomic_dec(&tbl->entries);
                }
                write_unlock(&svc->sched_lock);
        }

Shouldn't this be "time_before"?  It seems that it currently nukes all
recently-used entries every time this function is called, which seems to
be every 30 minutes, rather than removing the not-recently-used ones.

If my reading is correct, this patch should fix it.  Am I missing
something?

Cheers,

Simon-

[-- Attachment #2: lblcr+full_check_time_fix.patch --]
[-- Type: text/x-diff, Size: 535 bytes --]

diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
index 715b57f..937743f 100644
--- a/net/netfilter/ipvs/ip_vs_lblcr.c
+++ b/net/netfilter/ipvs/ip_vs_lblcr.c
@@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc)
 
 		write_lock(&svc->sched_lock);
 		list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {
-			if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
+			if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration,
 				       now))
 				continue;
 

Re: test

[Julian Anastasov]

	Hello,

On Wed, 4 Nov 2009, Simon Kirby wrote:

> Hello!
> 
> I was noticing a significant amount of what seems/seemed to be
> destination lists with multiple entries with the lblcr LVS algorithm. 
> While tracking it down, I think I stumbled over a mistake.  In
> ip_vs_lblcr_full_check(), it appears the time check logic is reversed:
> 
>         for (i=0, j=tbl->rover; i<IP_VS_LBLCR_TAB_SIZE; i++) {
>                 j = (j + 1) & IP_VS_LBLCR_TAB_MASK;
> 
>                 write_lock(&svc->sched_lock);
>                 list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) {

	If 'time to expire' is after current time then continue,
i.e. current time didn't reached the limit, seems correct,
no need to patch. For better reading and to match
ip_vs_lblcr_check_expire() it can be converted to:

if (time_before(now, en->lastuse+sysctl_ip_vs_lblcr_expiration))
	continue;


>                         if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration,
>                                        now))
>                                 continue;
>                         
>                         ip_vs_lblcr_free(en);
>                         atomic_dec(&tbl->entries);
>                 }
>                 write_unlock(&svc->sched_lock);
>         }
> 
> Shouldn't this be "time_before"?  It seems that it currently nukes all
> recently-used entries every time this function is called, which seems to
> be every 30 minutes, rather than removing the not-recently-used ones.
> 
> If my reading is correct, this patch should fix it.  Am I missing
> something?

include/linux/jiffies.h:

time_after(a,b) returns true if the time a is after time b.
#define time_before(a,b)        time_after(b,a)

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: test

[Simon Kirby]

On Thu, Nov 05, 2009 at 11:26:27AM +0200, Julian Anastasov wrote:

> 	If 'time to expire' is after current time then continue,
> i.e. current time didn't reached the limit, seems correct,
> no need to patch. For better reading and to match
> ip_vs_lblcr_check_expire() it can be converted to:
> 
> if (time_before(now, en->lastuse+sysctl_ip_vs_lblcr_expiration))
> 	continue;

D'oh.  I noticed the use of time_before() further down in
ip_vs_lblcr_check_expire(), but not the reversed arguments, hence my
confusion.

I still suspect there may be something not quite right, or which could
perhaps do with some tuning.  It's difficult to see exactly how it's
working internally, since there's currently nothing to get a summary of
the dest_sets to userspace.  I'll follow up if I find anything.

Simon-

[PATCH] taprio: Set the value of picos_per_byte before fill sched_entry

[jianghaoran]

If the value of picos_per_byte is set after fill sched_entry,
as a result, the min_duration calculated by length_to_duration is 0,
and the validity of the input interval cannot be judged,
too small intervals couldn't allow any packet to be transmitted.
It will appear like commit b5b73b26b3ca ("taprio:
Fix allowing too small intervals") described problem.
Here is a further modification of this problem.

example:

tc qdisc replace dev enp5s0f0 parent root handle 100 taprio \
              num_tc 3 \
              map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
              queues 1@0 1@1 2@2 \
              base-time  1528743495910289987 \
              sched-entry S 01 9 \
	      sched-entry S 02 9 \
	      sched-entry S 04 9 \
              clockid CLOCK_TAI

Signed-off-by: jianghaoran <jianghaoran@kylinos.cn>
---
 net/sched/sch_taprio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 86675a79da1e..d95ec2250f24 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1507,6 +1507,8 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		goto free_sched;
 	}
 
+	taprio_set_picos_per_byte(dev, q);
+
 	err = parse_taprio_schedule(q, tb, new_admin, extack);
 	if (err < 0)
 		goto free_sched;
@@ -1521,8 +1523,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	if (err < 0)
 		goto free_sched;
 
-	taprio_set_picos_per_byte(dev, q);
-
 	if (mqprio) {
 		err = netdev_set_num_tc(dev, mqprio->num_tc);
 		if (err)
-- 
2.25.1

Re: [PATCH] taprio: Set the value of picos_per_byte before fill sched_entry

[Jakub Kicinski]

On Wed, 28 Sep 2022 14:58:30 +0800 jianghaoran wrote:
> If the value of picos_per_byte is set after fill sched_entry,
> as a result, the min_duration calculated by length_to_duration is 0,
> and the validity of the input interval cannot be judged,
> too small intervals couldn't allow any packet to be transmitted.

Meaning an invalid configuration is accepted but no packets
can ever be transmitted?  Could you make the user-visible 
issue clearer?

> It will appear like commit b5b73b26b3ca ("taprio:
> Fix allowing too small intervals") described problem.
> Here is a further modification of this problem.
> 
> example:

Here as well it seems worthwhile to mention what this is an example of.
e.g. "example configuration which will not be able to transmit packets"

> tc qdisc replace dev enp5s0f0 parent root handle 100 taprio \
>               num_tc 3 \
>               map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \
>               queues 1@0 1@1 2@2 \
>               base-time  1528743495910289987 \
>               sched-entry S 01 9 \
> 	      sched-entry S 02 9 \
> 	      sched-entry S 04 9 \
>               clockid CLOCK_TAI

Please add a Fixes tag pointing to the first commit where the issue was
present, and CC Vladimir Oltean <vladimir.oltean@nxp.com> on the next
version.

test

[jianghaoran]

test

[Rain River]

Please ignore it.

test

[Ding Tianhong]

test

test

[lkernmnet]

tttttttttttttttttt

test

[Vlad Yasevich]

test

[Jacky Hsiao]