diff mbox

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

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

Commit Message

高峰 Nov. 21, 2016, 12:39 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 packe 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>
---
 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

Sven Eckelmann Nov. 21, 2016, 8:16 a.m. UTC | #1 
On Montag, 21. November 2016 08:39:39 CET 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 packe is lost. Other modules like ipvlan,

s/packe/packet/

> 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>
> ---
>  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(-)
[...]

Not sure how the batman-adv maintainers see this - but this patch needs
an additional patch for net-next to also add it to the parts which were
rewritten.

Kind regards,
	Sven
Feng Gao Nov. 21, 2016, 8:21 a.m. UTC | #2 
Hi Sven,

On Mon, Nov 21, 2016 at 4:16 PM, Sven Eckelmann <sven@narfation.org> wrote:
> On Montag, 21. November 2016 08:39:39 CET 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 packe is lost. Other modules like ipvlan,
>
> s/packe/packet/

What's this mean?

>
>> 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>
>> ---
>>  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(-)
> [...]
>
> Not sure how the batman-adv maintainers see this - but this patch needs
> an additional patch for net-next to also add it to the parts which were
> rewritten.
>
> Kind regards,
>         Sven

Ok. I would commit another patch to net-next.

Best Regards
Feng
Sven Eckelmann Nov. 21, 2016, 8:31 a.m. UTC | #3 
On Montag, 21. November 2016 16:21:52 CET Feng Gao wrote:
> Hi Sven,
> 
> On Mon, Nov 21, 2016 at 4:16 PM, Sven Eckelmann <sven@narfation.org> wrote:
> > On Montag, 21. November 2016 08:39:39 CET 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 packe is lost. Other modules like ipvlan,
> >
> > s/packe/packet/
> 
> What's this mean?

That there is a minor typo (*t* is missing) and this sed statement (when
applied only to the commit message) would fix it.

David already marked this patch as "Under Review" in his patchwork. So I would
guess that he will accept this patch and not the batman-adv maintainers. And
maybe he will fix this small typo - or maybe not.

Kind regards,
	Sven
Feng Gao Nov. 21, 2016, 8:47 a.m. UTC | #4 
Hi Sven,

On Mon, Nov 21, 2016 at 4:31 PM, Sven Eckelmann <sven@narfation.org> wrote:
> On Montag, 21. November 2016 16:21:52 CET Feng Gao wrote:
>> Hi Sven,
>>
>> On Mon, Nov 21, 2016 at 4:16 PM, Sven Eckelmann <sven@narfation.org> wrote:
>> > On Montag, 21. November 2016 08:39:39 CET 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 packe is lost. Other modules like ipvlan,
>> >
>> > s/packe/packet/
>>
>> What's this mean?
>
> That there is a minor typo (*t* is missing) and this sed statement (when
> applied only to the commit message) would fix it.

Thanks. I didn't thought it was sed statement.

>
> David already marked this patch as "Under Review" in his patchwork. So I would
> guess that he will accept this patch and not the batman-adv maintainers. And
> maybe he will fix this small typo - or maybe not.
>
> Kind regards,
>         Sven

I would correct the typo in the patch for net-next.

Best Regards
Feng
Sergei Shtylyov Nov. 21, 2016, 10:44 a.m. UTC | #5 
Hello.

On 11/21/2016 3:39 AM, 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 packe is lost. Other modules like ipvlan,

    Packet.

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

[...]

> diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
> index 7e8dc64..8edd324 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 || ret == NET_XMIT_CN) {

    Not '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,
[...]

MBR, Sergei
Feng Gao Nov. 21, 2016, 11:01 a.m. UTC | #6 
Hi Sergei,

On Mon, Nov 21, 2016 at 6:44 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 11/21/2016 3:39 AM, 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 packe is lost. Other modules like ipvlan,
>
>
>    Packet.

Thanks, it was typo.

>
>> 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>
>
>
> [...]
>
>> diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
>> index 7e8dc64..8edd324 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 || ret == NET_XMIT_CN) {

Thanks again.
I didn't find it during myself's review and compile process.

>
>
>    Not '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,
>
> [...]
>
> MBR, Sergei
>

I have sent the v2 patch which corrects these two typos.

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..8edd324 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 || ret == 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;