diff mbox

[net,v2,1/1] net: batman-adv: Treat NET_XMIT_CN as transmit successfully

Message ID 1479725922-5112-1-git-send-email-fgao@ikuai8.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

高峰 Nov. 21, 2016, 10:58 a.m. UTC 
  From: Gao Feng <fgao@ikuai8.com>

The tc could return NET_XMIT_CN as one congestion notification, but
it does not mean the packet is lost. Other modules like ipvlan,
macvlan, and others treat NET_XMIT_CN as success too.

So batman-adv should add the NET_XMIT_CN check.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 v2: Correct two typo "packe" and "ret"
 v1: Initial version

 net/batman-adv/distributed-arp-table.c | 2 +-
 net/batman-adv/fragmentation.c         | 2 +-
 net/batman-adv/routing.c               | 2 +-
 net/batman-adv/tp_meter.c              | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

Comments

Florian Westphal Nov. 21, 2016, 11:09 a.m. UTC | #1 
fgao@ikuai8.com <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
> 
> The tc could return NET_XMIT_CN as one congestion notification, but
> it does not mean the packet is lost. Other modules like ipvlan,
> macvlan, and others treat NET_XMIT_CN as success too.
> 
> So batman-adv should add the NET_XMIT_CN check.

"The tc could return NET_XMIT_CN as one congestion notification, but
it means another packet got dropped. Other modules like batman do not
treat NET_XMIT_CN as success, so modules like ipvlan, macvlan, ..
should ignore it as well."

What I am asking is:
Are you sure adding NET_XMIT_CN handling everywhere is the right way to
resolve the inconsistency?
高峰 Nov. 21, 2016, 1:24 p.m. UTC | #2 
Hi Florian,

On Mon, Nov 21, 2016 at 7:09 PM, Florian Westphal <fw@strlen.de> wrote:
> fgao@ikuai8.com <fgao@ikuai8.com> wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> The tc could return NET_XMIT_CN as one congestion notification, but
>> it does not mean the packet is lost. Other modules like ipvlan,
>> macvlan, and others treat NET_XMIT_CN as success too.
>>
>> So batman-adv should add the NET_XMIT_CN check.
>
> "The tc could return NET_XMIT_CN as one congestion notification, but
> it means another packet got dropped. Other modules like batman do not
> treat NET_XMIT_CN as success, so modules like ipvlan, macvlan, ..
> should ignore it as well."
>
> What I am asking is:
> Are you sure adding NET_XMIT_CN handling everywhere is the right way to
> resolve the inconsistency?

Or create one new macro to handle this case like net_xmit_eval.
For example,
#define net_xmit_ok(ret)    (ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)

Is it ok? But it should be done for net-next.

Best Regards
Feng
高峰 Nov. 21, 2016, 1:31 p.m. UTC | #3 
Hi Florian,


On Mon, Nov 21, 2016 at 9:24 PM, Gao Feng <fgao@ikuai8.com> wrote:
> Hi Florian,
>
> On Mon, Nov 21, 2016 at 7:09 PM, Florian Westphal <fw@strlen.de> wrote:
>> fgao@ikuai8.com <fgao@ikuai8.com> wrote:
>>> From: Gao Feng <fgao@ikuai8.com>
>>>
>>> The tc could return NET_XMIT_CN as one congestion notification, but
>>> it does not mean the packet is lost. Other modules like ipvlan,
>>> macvlan, and others treat NET_XMIT_CN as success too.
>>>
>>> So batman-adv should add the NET_XMIT_CN check.
>>
>> "The tc could return NET_XMIT_CN as one congestion notification, but
>> it means another packet got dropped. Other modules like batman do not
>> treat NET_XMIT_CN as success, so modules like ipvlan, macvlan, ..
>> should ignore it as well."

I didn't get you in the last email

You mean the NET_XMIT_CN should be treated as one error?
But the comment of NET_XMIT_CN says "It indicates that the device will
soon be dropping packets, or already drops some packets of the same
priority". It is not sure another packet is dropped.

BTW, the macro "net_xmit_eval" does not treat NET_XMIT_CN as one error.

Regards
Feng

>>
>> What I am asking is:
>> Are you sure adding NET_XMIT_CN handling everywhere is the right way to
>> resolve the inconsistency?
>
> Or create one new macro to handle this case like net_xmit_eval.
> For example,
> #define net_xmit_ok(ret)    (ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)
>
> Is it ok? But it should be done for net-next.
>
> Best Regards
> Feng
diff mbox

Patch

diff --git a/net/batman-adv/distributed-arp-table.c b/net/batman-adv/distributed-arp-table.c
index e257efd..4bf0622 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -660,7 +660,7 @@  static bool batadv_dat_send_data(struct batadv_priv *bat_priv,
 		}
 
 		send_status = batadv_send_unicast_skb(tmp_skb, neigh_node);
-		if (send_status == NET_XMIT_SUCCESS) {
+		if (send_status == NET_XMIT_SUCCESS || send_status == NET_XMIT_CN) {
 			/* count the sent packet */
 			switch (packet_subtype) {
 			case BATADV_P_DAT_DHT_GET:
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c
index 0934730..4714b8f 100644
--- a/net/batman-adv/fragmentation.c
+++ b/net/batman-adv/fragmentation.c
@@ -495,7 +495,7 @@  int batadv_frag_send_packet(struct sk_buff *skb,
 		batadv_add_counter(bat_priv, BATADV_CNT_FRAG_TX_BYTES,
 				   skb_fragment->len + ETH_HLEN);
 		ret = batadv_send_unicast_skb(skb_fragment, neigh_node);
-		if (ret != NET_XMIT_SUCCESS) {
+		if (ret != NET_XMIT_SUCCESS && ret != NET_XMIT_CN) {
 			/* return -1 so that the caller can free the original
 			 * skb
 			 */
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 7e8dc64..f44fb07 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -706,7 +706,7 @@  static int batadv_route_unicast_packet(struct sk_buff *skb,
 		goto out;
 
 	/* translate transmit result into receive result */
-	if (res == NET_XMIT_SUCCESS) {
+	if (res == NET_XMIT_SUCCESS || res == NET_XMIT_CN) {
 		/* skb was transmitted and consumed */
 		batadv_inc_counter(bat_priv, BATADV_CNT_FORWARD);
 		batadv_add_counter(bat_priv, BATADV_CNT_FORWARD_BYTES,
diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c
index 8af1611..461dbad 100644
--- a/net/batman-adv/tp_meter.c
+++ b/net/batman-adv/tp_meter.c
@@ -618,7 +618,7 @@  static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src,
 	if (r == -1)
 		kfree_skb(skb);
 
-	if (r == NET_XMIT_SUCCESS)
+	if (r == NET_XMIT_SUCCESS || r == NET_XMIT_CN)
 		return 0;
 
 	return BATADV_TP_REASON_CANT_SEND;