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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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.

On 2022/8/27 10:40, Jakub Kicinski wrote:

On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:

quoted

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

On Sat, 27 Aug 2022 11:16:53 +0800 shaozhengchao wrote:

On 2022/8/27 10:40, Jakub Kicinski wrote:

quoted

On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:  

quoted

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.

On 2022/8/30 12:48, Jakub Kicinski wrote:

On Sat, 27 Aug 2022 11:16:53 +0800 shaozhengchao wrote:

quoted

On 2022/8/27 10:40, Jakub Kicinski wrote:

quoted

On Thu, 25 Aug 2022 11:29:40 +0800 Zhengchao Shao wrote:

quoted

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