tcp: minor cleanup of syncache code

[tuexen]

Summary

Add a comment explaining why syncache entries are dropped, fix a typo in a comment and add a missing counter.

[glebius]

tcp_var.h is not source of truth here! This file and also netstat(1) are. In this file the counter only is updated when we indeed drop. In netstat counter is reported to user as "dropped". This change should be a comment only change.

[tuexen]

In D53564#1223369, @glebius wrote:

tcp_var.h is not source of truth here! This file and also netstat(1) are. In this file the counter only is updated when we indeed drop. In netstat counter is reported to user as "dropped". This change should be a comment only change.

Without this change, we increment tcps_sc_dropped three times:

1. In syncache_timer(), the pattern is

if (syncache_respond() {
	...
} else {
	syncache_drop(sc, sch);
	TCPSTAT_INC(tcps_sc_dropped);
}

2. In syncache_add(), the the first pattern is

if (syncache_respond() {
	...
} else {
	syncache_drop(sc, sch);
	TCPSTAT_INC(tcps_sc_dropped);
}

3. In syncache_add(), the the second pattern is

if (syncache_respond() {
	...
} else {
	if (sc != &scs)
		syncache_free(sc);
	TCPSTAT_INC(tcps_sc_dropped);
}

So tcps_sc_dropped is incremented, when syncache_response() fails. If it would count when we drop a syncache entry, we would need to change the last pattern. I guess it is more "the response was dropped", not "a syncache entry was dropped".
This patch just consistently uses the counter in the case syncache_response() fails, or if you prefer, the response is dropped.

[glebius]

In the third case we also drop. In the first two cases we called syncache_drop() that would unlink and free the entry. In the third case we have already allocated an entry and we were about to link it in, but we resulted in freeing it.

The suggested fourth case is in its nature different to the first three.

[glebius]

I see your logic, but then we need to reformat netstat, too. And that probably shouldn't get MFC-ed to stable/15.

2 syncache entries added
        0 retransmitted
        0 dupsyn
        0 dropped
        2 completed
        0 bucket overflow
        0 cache overflow
        0 reset
        0 stale
        0 aborted
        0 badack
        0 unreach
        0 zone failures
2 cookies sent
        0 cookies received
        0 spurious cookies rejected
        0 failed cookies rejected

We don't want dropped to grow as challenge ack sending fails.

[tuexen]

In D53564#1223411, @glebius wrote:

In the third case we also drop. In the first two cases we called syncache_drop() that would unlink and free the entry. In the third case we have already allocated an entry and we were about to link it in, but we resulted in freeing it.

The third case is only a drop, if we have allocated an entry. If we are using syncookies, we also increment the counter if the syncache_response() call failed.

The suggested fourth case is in its nature different to the first three.

This depends on the semantic of the counter:

• if the semantic is "reply is dropped", it is the same and it should be incremented.
• if the semantic is "syncache entry is dropped", it is not the same and the third pattern should be changed to increment the counter only if we are actually calling syncache_free().

[tuexen]

In D53564#1223412, @glebius wrote:

I see your logic, but then we need to reformat netstat, too. And that probably shouldn't get MFC-ed to stable/15.

2 syncache entries added
        0 retransmitted
        0 dupsyn
        0 dropped
        2 completed
        0 bucket overflow
        0 cache overflow
        0 reset
        0 stale
        0 aborted
        0 badack
        0 unreach
        0 zone failures
2 cookies sent
        0 cookies received
        0 spurious cookies rejected
        0 failed cookies rejected

We don't want dropped to grow as challenge ack sending fails.

Maybe we want separate counters for:

• syncache_repsonse() failed
• syncache entries are dropped.

I only look consistent counters enabling me to understand what happened.
If we go for two counters, we would use something like:

	if (syncache_respond(sc, m, TH_SYN|TH_ACK) == 0) {
		if (sc != &scs)
			syncache_insert(sc, sch);   /* locks and unlocks sch */
		TCPSTAT_INC(tcps_sndacks);
		TCPSTAT_INC(tcps_sndtotal);
	} else {
		if (sc != &scs) {
			syncache_free(sc);
			 TCPSTAT_INC(tcps_sc_dropped);
		}
		TCPSTAT_INC(tcps_sc_respsonse_failed;
	}

I hope this makes it clearer...

[glebius]

In D53564#1223423, @tuexen wrote:

Maybe we want separate counters for:

• syncache_repsonse() failed
• syncache entries are dropped.

Yes, this is the way to go! Can you please commit this revision as comment change only and create a new one that would make two counters and improve descriptions in netstat?

[tuexen]

In D53564#1223644, @glebius wrote:

In D53564#1223423, @tuexen wrote:

Maybe we want separate counters for:

• syncache_repsonse() failed
• syncache entries are dropped.

Yes, this is the way to go! Can you please commit this revision as comment change only and create a new one that would make two counters and improve descriptions in netstat?

Will do.

[tuexen]

Only improve comments.