[PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[Zhengchao Shao]

According to the description, "other" should be added when calling
qdisc_drop() to discard packets.

Zhengchao Shao (3):
  net: sched: sch_choke: add statistics when calling qdisc_drop() in
    sch_choke
  net: sched: sch_gred: add statistics when calling qdisc_drop() in
    sch_gred
  net: sched: sch_red: add statistics when calling qdisc_drop() in
    sch_red

 include/net/red.h     | 2 +-
 net/sched/sch_choke.c | 5 ++++-
 net/sched/sch_gred.c  | 2 ++
 net/sched/sch_red.c   | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] net: sched: sch_choke: add statistics when calling qdisc_drop() in sch_choke

[Zhengchao Shao]

Now, the "other" member in the choke_sched_data structure is not used.
According to the description, "other" should be added when calling
qdisc_drop() to discard packets.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/sched/sch_choke.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index 2adbd945bf15..19c25ec36d0d 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -60,7 +60,7 @@ struct choke_sched_data {
 		u32	forced_drop;	/* Forced drops, qavg > max_thresh */
 		u32	forced_mark;	/* Forced marks, qavg > max_thresh */
 		u32	pdrop;          /* Drops due to queue limits */
-		u32	other;          /* Drops due to drop() calls */
+		u32	other;          /* Drops due to qdisc_drop() calls */
 		u32	matched;	/* Drops to flow match */
 	} stats;
 
@@ -127,6 +127,7 @@ static void choke_drop_by_idx(struct Qdisc *sch, unsigned int idx,
 	qdisc_qstats_backlog_dec(sch, skb);
 	qdisc_tree_reduce_backlog(sch, 1, qdisc_pkt_len(skb));
 	qdisc_drop(skb, sch, to_free);
+	q->stats.other++;
 	--sch->q.qlen;
 }
 
@@ -274,9 +275,11 @@ static int choke_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 	q->stats.pdrop++;
+	q->stats.other++;
 	return qdisc_drop(skb, sch, to_free);
 
 congestion_drop:
+	q->stats.other++;
 	qdisc_drop(skb, sch, to_free);
 	return NET_XMIT_CN;
 }
-- 
2.17.1

[PATCH net-next 2/3] net: sched: sch_gred: add statistics when calling qdisc_drop() in sch_gred

[Zhengchao Shao]

Now, the "other" member in the gred_sched_data structure is not used.
According to the description, "other" should be added when calling
qdisc_drop() to discard packets.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 include/net/red.h    | 2 +-
 net/sched/sch_gred.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/net/red.h b/include/net/red.h
index 30c6a23ab8cc..dad41eff8c62 100644
--- a/include/net/red.h
+++ b/include/net/red.h
@@ -122,7 +122,7 @@ struct red_stats {
 	u32		forced_drop;	/* Forced drops, qavg > max_thresh */
 	u32		forced_mark;	/* Forced marks, qavg > max_thresh */
 	u32		pdrop;          /* Drops due to queue limits */
-	u32		other;          /* Drops due to drop() calls */
+	u32		other;          /* Drops due to qdisc_drop() calls */
 };
 
 struct red_parms {
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index 1073c76d05c4..c50a0853dcb9 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -251,9 +251,11 @@ static int gred_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 
 	q->stats.pdrop++;
 drop:
+	q->stats.other++;
 	return qdisc_drop(skb, sch, to_free);
 
 congestion_drop:
+	q->stats.other++;
 	qdisc_drop(skb, sch, to_free);
 	return NET_XMIT_CN;
 }
-- 
2.17.1

[PATCH net-next 3/3] net: sched: sch_red: add statistics when calling qdisc_drop() in sch_red

[Zhengchao Shao]

Now, the "other" member in the red_sched_data structure is not used.
According to the description, "other" should be added when calling
qdisc_drop() to discard packets.

Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
---
 net/sched/sch_red.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 40adf1f07a82..cdf9d8611e41 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -141,6 +141,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	if (!skb)
 		return NET_XMIT_CN | ret;
 
+	q->stats.other++;
 	qdisc_drop(skb, sch, to_free);
 	return NET_XMIT_CN;
 }
-- 
2.17.1

Re: [PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[Jakub Kicinski]

On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:
> According to the description, "other" should be added when calling
> qdisc_drop() to discard packets.

The fact that an old copy & pasted comment says something is not 
in itself a sufficient justification to make code changes.

qdisc_drop() already counts drops, duplicating the same information 
in another place seems like a waste of CPU cycles.

Re: [PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[shaozhengchao]

On 2022/8/27 10:40, Jakub Kicinski wrote:
> On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:
>> According to the description, "other" should be added when calling
>> qdisc_drop() to discard packets.
> 
> The fact that an old copy & pasted comment says something is not
> in itself a sufficient justification to make code changes.
> 
> qdisc_drop() already counts drops, duplicating the same information
> in another place seems like a waste of CPU cycles.

Hi Jakub:
	Thank you for your reply. It seems more appropriate to delete the other 
variable, if it is unused?

Zhengchao Shao

Re: [PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[Jakub Kicinski]

On Sat, 27 Aug 2022 11:16:53 +0800 shaozhengchao wrote:
> On 2022/8/27 10:40, Jakub Kicinski wrote:
> > On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:  
> >> According to the description, "other" should be added when calling
> >> qdisc_drop() to discard packets.  
> > 
> > The fact that an old copy & pasted comment says something is not
> > in itself a sufficient justification to make code changes.
> > 
> > qdisc_drop() already counts drops, duplicating the same information
> > in another place seems like a waste of CPU cycles.  
> 
> Hi Jakub:
> 	Thank you for your reply. It seems more appropriate to delete the other 
> variable, if it is unused?

Yes, removing it SGTM.

Re: [PATCH net-next 0/3] net: sched: add other statistics when calling qdisc_drop()

[shaozhengchao]

On 2022/8/30 12:48, Jakub Kicinski wrote:
> On Sat, 27 Aug 2022 11:16:53 +0800 shaozhengchao wrote:
>> On 2022/8/27 10:40, Jakub Kicinski wrote:
>>> On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:
>>>> According to the description, "other" should be added when calling
>>>> qdisc_drop() to discard packets.
>>>
>>> The fact that an old copy & pasted comment says something is not
>>> in itself a sufficient justification to make code changes.
>>>
>>> qdisc_drop() already counts drops, duplicating the same information
>>> in another place seems like a waste of CPU cycles.
>>
>> Hi Jakub:
>> 	Thank you for your reply. It seems more appropriate to delete the other
>> variable, if it is unused?
> 
> Yes, removing it SGTM.

Hi Jakub:
	Thank you. I have send v3.

Zhengchao Shao