* Re: [PATCH V2] tipc: Use bsearch library function
From: Joe Perches @ 2017-09-16 9:26 UTC (permalink / raw)
To: Ying Xue, Thomas Meyer, jon.maloy, netdev, tipc-discussion,
linux-kernel, davem
In-Reply-To: <16128f5e-66ff-b6ec-c0e1-74ea08c212b0@windriver.com>
On Sat, 2017-09-16 at 17:02 +0800, Ying Xue wrote:
> On 09/16/2017 03:50 PM, Thomas Meyer wrote:
> > Use common library function rather than explicitly coding
> > some variant of it yourself.
> >
> > Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
>
> Acked-by: Ying Xue <ying.xue@windriver.com>
Are you sure you want to do this?
Note the comment above nameseq_find_subseq
* Very time-critical, so binary searches through sub-sequence array.
What impact does this change have on performance?
> > diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> > index bd0aac87b41a..eeb4d7a13de2 100644
> > --- a/net/tipc/name_table.c
> > +++ b/net/tipc/name_table.c
> > @@ -44,6 +44,7 @@
> > #include "addr.h"
> > #include "node.h"
> > #include <net/genetlink.h>
> > +#include <linux/bsearch.h>
> >
> > #define TIPC_NAMETBL_SIZE 1024 /* must be a power of 2 */
> >
> > @@ -168,6 +169,18 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
> > return nseq;
> > }
> >
> > +static int nameseq_find_subseq_cmp(const void *key, const void *elt)
> > +{
> > + struct sub_seq *sseq = (struct sub_seq *)elt;
> > + u32 instance = *(u32 *)key;
> > +
> > + if (instance < sseq->lower)
> > + return -1;
> > + else if (instance > sseq->upper)
> > + return 1;
> > + return 0;
> > +}
> > +
> > /**
> > * nameseq_find_subseq - find sub-sequence (if any) matching a name instance
> > *
> > @@ -176,21 +189,8 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
> > static struct sub_seq *nameseq_find_subseq(struct name_seq *nseq,
> > u32 instance)
> > {
> > - struct sub_seq *sseqs = nseq->sseqs;
> > - int low = 0;
> > - int high = nseq->first_free - 1;
> > - int mid;
> > -
> > - while (low <= high) {
> > - mid = (low + high) / 2;
> > - if (instance < sseqs[mid].lower)
> > - high = mid - 1;
> > - else if (instance > sseqs[mid].upper)
> > - low = mid + 1;
> > - else
> > - return &sseqs[mid];
> > - }
> > - return NULL;
> > + return bsearch(&instance, nseq->sseqs, nseq->first_free,
> > + sizeof(struct sub_seq), nameseq_find_subseq_cmp);
> > }
> >
> > /**
> >
^ permalink raw reply
* Re: [PATCH V2] tipc: Use bsearch library function
From: Ying Xue @ 2017-09-16 9:02 UTC (permalink / raw)
To: Thomas Meyer, jon.maloy, netdev, tipc-discussion, linux-kernel,
davem
In-Reply-To: <20170916075036.28676-1-thomas@m3y3r.de>
On 09/16/2017 03:50 PM, Thomas Meyer wrote:
> Use common library function rather than explicitly coding
> some variant of it yourself.
>
> Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
Acked-by: Ying Xue <ying.xue@windriver.com>
> ---
> net/tipc/name_table.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> V2: Coding style
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index bd0aac87b41a..eeb4d7a13de2 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -44,6 +44,7 @@
> #include "addr.h"
> #include "node.h"
> #include <net/genetlink.h>
> +#include <linux/bsearch.h>
>
> #define TIPC_NAMETBL_SIZE 1024 /* must be a power of 2 */
>
> @@ -168,6 +169,18 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
> return nseq;
> }
>
> +static int nameseq_find_subseq_cmp(const void *key, const void *elt)
> +{
> + struct sub_seq *sseq = (struct sub_seq *)elt;
> + u32 instance = *(u32 *)key;
> +
> + if (instance < sseq->lower)
> + return -1;
> + else if (instance > sseq->upper)
> + return 1;
> + return 0;
> +}
> +
> /**
> * nameseq_find_subseq - find sub-sequence (if any) matching a name instance
> *
> @@ -176,21 +189,8 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
> static struct sub_seq *nameseq_find_subseq(struct name_seq *nseq,
> u32 instance)
> {
> - struct sub_seq *sseqs = nseq->sseqs;
> - int low = 0;
> - int high = nseq->first_free - 1;
> - int mid;
> -
> - while (low <= high) {
> - mid = (low + high) / 2;
> - if (instance < sseqs[mid].lower)
> - high = mid - 1;
> - else if (instance > sseqs[mid].upper)
> - low = mid + 1;
> - else
> - return &sseqs[mid];
> - }
> - return NULL;
> + return bsearch(&instance, nseq->sseqs, nseq->first_free,
> + sizeof(struct sub_seq), nameseq_find_subseq_cmp);
> }
>
> /**
>
^ permalink raw reply
* [PATCH V2] tipc: Use bsearch library function
From: Thomas Meyer @ 2017-09-16 7:50 UTC (permalink / raw)
To: jon.maloy, ying.xue, netdev, tipc-discussion, linux-kernel, davem
Cc: Thomas Meyer
In-Reply-To: <20170911.143025.555018840006192902.davem@davemloft.net>
Use common library function rather than explicitly coding
some variant of it yourself.
Signed-off-by: Thomas Meyer <thomas@m3y3r.de>
---
net/tipc/name_table.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
V2: Coding style
diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index bd0aac87b41a..eeb4d7a13de2 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -44,6 +44,7 @@
#include "addr.h"
#include "node.h"
#include <net/genetlink.h>
+#include <linux/bsearch.h>
#define TIPC_NAMETBL_SIZE 1024 /* must be a power of 2 */
@@ -168,6 +169,18 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
return nseq;
}
+static int nameseq_find_subseq_cmp(const void *key, const void *elt)
+{
+ struct sub_seq *sseq = (struct sub_seq *)elt;
+ u32 instance = *(u32 *)key;
+
+ if (instance < sseq->lower)
+ return -1;
+ else if (instance > sseq->upper)
+ return 1;
+ return 0;
+}
+
/**
* nameseq_find_subseq - find sub-sequence (if any) matching a name instance
*
@@ -176,21 +189,8 @@ static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_hea
static struct sub_seq *nameseq_find_subseq(struct name_seq *nseq,
u32 instance)
{
- struct sub_seq *sseqs = nseq->sseqs;
- int low = 0;
- int high = nseq->first_free - 1;
- int mid;
-
- while (low <= high) {
- mid = (low + high) / 2;
- if (instance < sseqs[mid].lower)
- high = mid - 1;
- else if (instance > sseqs[mid].upper)
- low = mid + 1;
- else
- return &sseqs[mid];
- }
- return NULL;
+ return bsearch(&instance, nseq->sseqs, nseq->first_free,
+ sizeof(struct sub_seq), nameseq_find_subseq_cmp);
}
/**
--
2.11.0
^ permalink raw reply related
* Re: [Outreachy kernel] [PATCH] Staging: irda: Use !x instead of NULL comparison
From: Julia Lawall @ 2017-09-16 7:14 UTC (permalink / raw)
To: Srishti Sharma
Cc: samuel, gregkh, netdev, devel, linux-kernel, outreachy-kernel
In-Reply-To: <1505543667-4670-1-git-send-email-srishtishar@gmail.com>
On Sat, 16 Sep 2017, Srishti Sharma wrote:
> Test for NULL as !x where functions that return NULL on failure
> are used. Done using the following semantic patch by coccinelle.
>
> @ is_null @
> expression E;
> statement S;
> @@
>
> E = (\(kmalloc\|devm_kzalloc\|kmalloc_array\|devm_ioremap\|
> usb_alloc_urb\|alloc_netdev\|dev_alloc_skb\)(...));
>
> (
> if(!E)
> S
> |
> -if(E==NULL)
> +if(!E)
> S
> )
>
> Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
> ---
> drivers/staging/irda/net/discovery.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/irda/net/discovery.c b/drivers/staging/irda/net/discovery.c
> index 364d70a..1e54954 100644
> --- a/drivers/staging/irda/net/discovery.c
> +++ b/drivers/staging/irda/net/discovery.c
> @@ -179,7 +179,7 @@ void irlmp_expire_discoveries(hashbin_t *log, __u32 saddr, int force)
> /* Create the client specific buffer */
> n = HASHBIN_GET_SIZE(log);
> buffer = kmalloc(n * sizeof(struct irda_device_info), GFP_ATOMIC);
> - if (buffer == NULL) {
> + if (!buffer) {
> spin_unlock_irqrestore(&log->hb_spinlock, flags);
> return;
> }
> @@ -291,7 +291,7 @@ struct irda_device_info *irlmp_copy_discoveries(hashbin_t *log, int *pn,
> /* Create the client specific buffer */
> n = HASHBIN_GET_SIZE(log);
> buffer = kmalloc(n * sizeof(struct irda_device_info), GFP_ATOMIC);
> - if (buffer == NULL) {
> + if (!buffer) {
> spin_unlock_irqrestore(&log->hb_spinlock, flags);
> return NULL;
> }
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1505543667-4670-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>
^ permalink raw reply
* [PATCH] Staging: irda: Use !x instead of NULL comparison
From: Srishti Sharma @ 2017-09-16 6:34 UTC (permalink / raw)
To: samuel
Cc: gregkh, netdev, devel, linux-kernel, outreachy-kernel,
Srishti Sharma
Test for NULL as !x where functions that return NULL on failure
are used. Done using the following semantic patch by coccinelle.
@ is_null @
expression E;
statement S;
@@
E = (\(kmalloc\|devm_kzalloc\|kmalloc_array\|devm_ioremap\|
usb_alloc_urb\|alloc_netdev\|dev_alloc_skb\)(...));
(
if(!E)
S
|
-if(E==NULL)
+if(!E)
S
)
Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
---
drivers/staging/irda/net/discovery.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/irda/net/discovery.c b/drivers/staging/irda/net/discovery.c
index 364d70a..1e54954 100644
--- a/drivers/staging/irda/net/discovery.c
+++ b/drivers/staging/irda/net/discovery.c
@@ -179,7 +179,7 @@ void irlmp_expire_discoveries(hashbin_t *log, __u32 saddr, int force)
/* Create the client specific buffer */
n = HASHBIN_GET_SIZE(log);
buffer = kmalloc(n * sizeof(struct irda_device_info), GFP_ATOMIC);
- if (buffer == NULL) {
+ if (!buffer) {
spin_unlock_irqrestore(&log->hb_spinlock, flags);
return;
}
@@ -291,7 +291,7 @@ struct irda_device_info *irlmp_copy_discoveries(hashbin_t *log, int *pn,
/* Create the client specific buffer */
n = HASHBIN_GET_SIZE(log);
buffer = kmalloc(n * sizeof(struct irda_device_info), GFP_ATOMIC);
- if (buffer == NULL) {
+ if (!buffer) {
spin_unlock_irqrestore(&log->hb_spinlock, flags);
return NULL;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Florian Fainelli @ 2017-09-16 6:04 UTC (permalink / raw)
To: rosenp, Eric Dumazet; +Cc: netdev
In-Reply-To: <1505522322.3726.0.camel@gmail.com>
On September 15, 2017 5:38:42 PM PDT, rosenp@gmail.com wrote:
>I have not. Unfortunately I own no gigabit hardware to test this on.
>The MIPS CPU runs at 300MHz on my unit.
>
bgmac is used on Gigabit capable hardware, like Northstar and Northstar Plus, and others too, so unless you can get access to such HW or get confirmation from someone that your patches changes something, I would just drop this change and not bother. This is already not 100mbits/sec linerate...
>On Fri, 2017-09-15 at 17:34 -0700, Eric Dumazet wrote:
>> On Fri, 2017-09-15 at 17:23 -0700, Rosen Penev wrote:
>> > On a linksys E1200v1 (actually a crossflashed E1000v2), the
>> > offloading features give no measurable benefit to speed or latency.
>> > Furthermore, disabling GRO actually improves iperf performance by a
>> > whoppimg 3mbps. Results:
>> >
>> > Currently:
>> >
>> > v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an
>> > identical result.
>> >
>> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>> > ---
>> > drivers/net/ethernet/broadcom/bgmac.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c
>> > b/drivers/net/ethernet/broadcom/bgmac.c
>> > index 48d672b204a4..1fb0053aeee7 100644
>> > --- a/drivers/net/ethernet/broadcom/bgmac.c
>> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> > @@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac
>> > *bgmac, struct bgmac_dma_ring *ring,
>> > skb->protocol = eth_type_trans(skb, bgmac-
>> > >net_dev);
>> > bgmac->net_dev->stats.rx_bytes += len;
>> > bgmac->net_dev->stats.rx_packets++;
>> > - napi_gro_receive(&bgmac->napi, skb);
>> > + netif_receive_skb(skb);
>> > handled++;
>> > } while (0);
>> >
>>
>> And have you tested 1Gbit link speed ?
>> ( Or 2.5 Gbit link speed )
>>
>> If you want to disable GRO on your host, fine : you can use ethtool
>> -K
>>
>>
>>
(please don't top-post)
--
Florian
^ permalink raw reply
* Re: [PATCH] once: switch to new jump label API
From: Eric Biggers @ 2017-09-16 4:07 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: David S. Miller, netdev, linux-kernel, Jason Baron,
Peter Zijlstra, Eric Biggers
In-Reply-To: <874lszmoh2.fsf@stressinduktion.org>
On Tue, Aug 22, 2017 at 02:44:41PM -0400, Hannes Frederic Sowa wrote:
> Eric Biggers <ebiggers3@gmail.com> writes:
>
> > From: Eric Biggers <ebiggers@google.com>
> >
> > Switch the DO_ONCE() macro from the deprecated jump label API to the new
> > one. The new one is more readable, and for DO_ONCE() it also makes the
> > generated code more icache-friendly: now the one-time initialization
> > code is placed out-of-line at the jump target, rather than at the inline
> > fallthrough case.
> >
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org.
>
> Thanks!
Great! Who though is the maintainer for this code? It seems it was originally
taken by David Miller through the networking tree. David, are you taking
further patches to the "once" functions, or should I be trying to get this into
-mm, or somewhere else?
Eric
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: David Miller @ 2017-09-16 3:56 UTC (permalink / raw)
To: eric.dumazet; +Cc: f.fainelli, rosenp, netdev, zajec5, nbd
In-Reply-To: <1505520210.29839.7.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 Sep 2017 17:03:30 -0700
> On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
>> On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com>
>> wrote:
>> >On a linksys E1200v1 (actually a crossflashed E1000v2), the
>> offloading
>> >features give no measurable benefit to speed or latency. Furthermore,
>> >disabling GRO actually improves iperf performance by a whoppimg
>> 3mbps.
>>
>> Do you have a way to generate gigabit tests and see what results you
>> are getting? We probably are not going to see a 30% improvement just
>> by extrapolation.
>>
> +1
>
> It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
> driver to handle these features.
>
> And of course GRO was not removed, meaning the bench results were non
> conclusive.
My sentiments exactly, I think this is a completely unwise change.
^ permalink raw reply
* Re: [PATCH net] tcp: fix data delivery rate
From: Soheil Hassas Yeganeh @ 2017-09-16 1:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, liujian, Eric Dumazet, Yuchung Cheng, Jerry Chu,
netdev, weiyongjun (A), wangkefeng 00227729
In-Reply-To: <1505519262.29839.5.camel@edumazet-glaptop3.roam.corp.google.com>
On Fri, Sep 15, 2017 at 7:47 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Now skb->mstamp_skb is updated later, we also need to call
> tcp_rate_skb_sent() after the update is done.
>
> Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Nice catch!
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1505522096.29839.13.camel@edumazet-glaptop3.roam.corp.google.com>
I have not. Unfortunately I own no gigabit hardware to test this on.
The MIPS CPU runs at 300MHz on my unit.
On Fri, 2017-09-15 at 17:34 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:23 -0700, Rosen Penev wrote:
> > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> > offloading features give no measurable benefit to speed or latency.
> > Furthermore, disabling GRO actually improves iperf performance by a
> > whoppimg 3mbps. Results:
> >
> > Currently:
> >
> > v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an
> > identical result.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> > ---
> > drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c
> > b/drivers/net/ethernet/broadcom/bgmac.c
> > index 48d672b204a4..1fb0053aeee7 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> > @@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac
> > *bgmac, struct bgmac_dma_ring *ring,
> > skb->protocol = eth_type_trans(skb, bgmac-
> > >net_dev);
> > bgmac->net_dev->stats.rx_bytes += len;
> > bgmac->net_dev->stats.rx_packets++;
> > - napi_gro_receive(&bgmac->napi, skb);
> > + netif_receive_skb(skb);
> > handled++;
> > } while (0);
> >
>
> And have you tested 1Gbit link speed ?
> ( Or 2.5 Gbit link speed )
>
> If you want to disable GRO on your host, fine : you can use ethtool
> -K
>
>
>
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:34 UTC (permalink / raw)
To: Rosen Penev; +Cc: netdev
In-Reply-To: <20170916002305.2794-1-rosenp@gmail.com>
On Fri, 2017-09-15 at 17:23 -0700, Rosen Penev wrote:
> On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
>
> Currently:
>
> v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an identical result.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
> drivers/net/ethernet/broadcom/bgmac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 48d672b204a4..1fb0053aeee7 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
> skb->protocol = eth_type_trans(skb, bgmac->net_dev);
> bgmac->net_dev->stats.rx_bytes += len;
> bgmac->net_dev->stats.rx_packets++;
> - napi_gro_receive(&bgmac->napi, skb);
> + netif_receive_skb(skb);
> handled++;
> } while (0);
>
And have you tested 1Gbit link speed ?
( Or 2.5 Gbit link speed )
If you want to disable GRO on your host, fine : you can use ethtool -K
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Denys Fedoryshchenko @ 2017-09-16 0:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: rosenp, Florian Fainelli, netdev, zajec5, nbd, netdev-owner
In-Reply-To: <1505521095.29839.11.camel@edumazet-glaptop3.roam.corp.google.com>
On 2017-09-16 03:18, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
>> Ok fair enough. Will only disable GRO in the driver.
>
> Well, do not even try.
>
> NETIF_F_SOFT_FEATURES is set by core networking stack in
> register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
> )
>
> Absolutely no driver disables GRO (excepts the ones playing with LRO)
I believe also iperf is definitely inconclusive test.
Except iperf there is lot of different workloads and configurations,
that might have different results.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Fainelli, netdev, zajec5, nbd
In-Reply-To: <1505521095.29839.11.camel@edumazet-glaptop3.roam.corp.google.com>
netif_receive_skb seems to have the same effect as "ethtool -K eth0 gro
off".
On Fri, 2017-09-15 at 17:18 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
> > Ok fair enough. Will only disable GRO in the driver.
>
> Well, do not even try.
>
> NETIF_F_SOFT_FEATURES is set by core networking stack in
> register_netdevice(), ( commit
> 212b573f5552c60265da721ff9ce32e3462a2cdd
> )
>
> Absolutely no driver disables GRO (excepts the ones playing with LRO)
>
>
>
>
>
>
^ permalink raw reply
* [PATCH] bgmac: Remove all offloading features, including GRO.
From: Rosen Penev @ 2017-09-16 0:23 UTC (permalink / raw)
To: netdev; +Cc: Rosen Penev
On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
Currently:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52410
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 641 75:181:12:1:1:0:0:371
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52412
[ 5] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 629 51:194:13:1:0:1:0:369
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52414
[ 4] 0.00-10.02 sec 51.9 MBytes 43.4 Mbits/sec 695 126:203:1:0:0:0:2:363
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52416
[ 5] 0.00-10.01 sec 52.4 MBytes 43.9 Mbits/sec 626 57:186:10:0:0:0:0:373
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52420
[ 4] 0.00-10.02 sec 52.4 MBytes 43.8 Mbits/sec 605 36:179:16:1:0:1:0:372
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52422
After disabling everything - including GRO:
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52440
[ ID] Interval Transfer Bandwidth Reads Dist(bin=16.0K)
[ 4] 0.00-10.01 sec 55.1 MBytes 46.2 Mbits/sec 672 180:82:0:0:1:0:0:409
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52442
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 636 117:96:0:0:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52444
[ 4] 0.00-10.01 sec 55.4 MBytes 46.4 Mbits/sec 675 172:92:0:0:1:0:0:410
[ 5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52446
[ 5] 0.00-10.01 sec 56.0 MBytes 46.9 Mbits/sec 633 119:90:0:1:1:0:0:422
[ 4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52448
[ 4] 0.00-10.01 sec 55.2 MBytes 46.3 Mbits/sec 688 157:123:0:0:2:0:0:406
v2: Changed napi_gro_receive to netif_receive_skb. Seems to have an identical result.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
drivers/net/ethernet/broadcom/bgmac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 48d672b204a4..1fb0053aeee7 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -483,7 +483,7 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
skb->protocol = eth_type_trans(skb, bgmac->net_dev);
bgmac->net_dev->stats.rx_bytes += len;
bgmac->net_dev->stats.rx_packets++;
- napi_gro_receive(&bgmac->napi, skb);
+ netif_receive_skb(skb);
handled++;
} while (0);
--
2.13.5
^ permalink raw reply related
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:18 UTC (permalink / raw)
To: rosenp; +Cc: Florian Fainelli, netdev, zajec5, nbd
In-Reply-To: <1505520638.8507.2.camel@gmail.com>
On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:
> Ok fair enough. Will only disable GRO in the driver.
Well, do not even try.
NETIF_F_SOFT_FEATURES is set by core networking stack in
register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
)
Absolutely no driver disables GRO (excepts the ones playing with LRO)
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-16 0:10 UTC (permalink / raw)
To: Eric Dumazet, Florian Fainelli; +Cc: netdev, zajec5, nbd
In-Reply-To: <1505520210.29839.7.camel@edumazet-glaptop3.roam.corp.google.com>
Ok fair enough. Will only disable GRO in the driver.
Tests were done using "ethtool -K eth0 gro off" and on.
On Fri, 2017-09-15 at 17:03 -0700, Eric Dumazet wrote:
> On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
> > On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com
> > >
> > wrote:
> > > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> >
> > offloading
> > > features give no measurable benefit to speed or latency.
> > > Furthermore,
> > > disabling GRO actually improves iperf performance by a whoppimg
> >
> > 3mbps.
> >
> > Do you have a way to generate gigabit tests and see what results
> > you
> > are getting? We probably are not going to see a 30% improvement
> > just
> > by extrapolation.
> >
>
> +1
>
> It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
> driver to handle these features.
>
> And of course GRO was not removed, meaning the bench results were non
> conclusive.
>
>
>
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Eric Dumazet @ 2017-09-16 0:03 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Rosen Penev, netdev, zajec5, nbd
In-Reply-To: <252FFED5-906E-4D1B-9E87-679D0CE801D4@gmail.com>
On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:
> On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com>
> wrote:
> >On a linksys E1200v1 (actually a crossflashed E1000v2), the
> offloading
> >features give no measurable benefit to speed or latency. Furthermore,
> >disabling GRO actually improves iperf performance by a whoppimg
> 3mbps.
>
> Do you have a way to generate gigabit tests and see what results you
> are getting? We probably are not going to see a 30% improvement just
> by extrapolation.
>
+1
It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
driver to handle these features.
And of course GRO was not removed, meaning the bench results were non
conclusive.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: rosenp @ 2017-09-15 23:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, zajec5, nbd
In-Reply-To: <20170915.161403.2131524326226213839.davem@davemloft.net>
you're absolutely correct. will send an updated version shortly.
On Fri, 2017-09-15 at 16:14 -0700, David Miller wrote:
> From: Rosen Penev <rosenp@gmail.com>
> Date: Fri, 15 Sep 2017 15:22:18 -0700
>
> > On a linksys E1200v1 (actually a crossflashed E1000v2), the
> offloading features give no measurable benefit to speed or latency.
> Furthermore, disabling GRO actually improves iperf performance by a
> whoppimg 3mbps. Results:
> ...
> > - net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM;
> > + net_dev->features &= NETIF_F_GRO;
> > net_dev->hw_features = net_dev->features;
>
> This doesn't disable GRO.
^ permalink raw reply
* [PATCH net] tcp: fix data delivery rate
From: Eric Dumazet @ 2017-09-15 23:47 UTC (permalink / raw)
To: David Miller
Cc: liujian56, edumazet, ycheng, hkchu, netdev, weiyongjun1,
wangkefeng.wang
In-Reply-To: <1505517411.29839.1.camel@edumazet-glaptop3.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Now skb->mstamp_skb is updated later, we also need to call
tcp_rate_skb_sent() after the update is done.
Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_output.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a85a8c2948e54b931f8cd956aa7938f7efd355bd..1c839c99114cd22bbfbd181cf702acccb3aeb61b 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1002,8 +1002,6 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
if (clone_it) {
TCP_SKB_CB(skb)->tx.in_flight = TCP_SKB_CB(skb)->end_seq
- tp->snd_una;
- tcp_rate_skb_sent(sk, skb);
-
oskb = skb;
if (unlikely(skb_cloned(skb)))
skb = pskb_copy(skb, gfp_mask);
@@ -1128,9 +1126,10 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
tcp_enter_cwr(sk);
err = net_xmit_eval(err);
}
- if (!err && oskb)
+ if (!err && oskb) {
oskb->skb_mstamp = tp->tcp_mstamp;
-
+ tcp_rate_skb_sent(sk, oskb);
+ }
return err;
}
^ permalink raw reply related
* Re: Use after free in __dst_destroy_metrics_generic
From: Subash Abhinov Kasiviswanathan @ 2017-09-15 23:20 UTC (permalink / raw)
To: Julian Anastasov
Cc: Eric Dumazet, Cong Wang, Linux Kernel Network Developers,
Lorenzo Colitti
In-Reply-To: <alpine.LFD.2.20.1709160103190.1770@ja.home.ssi.bg>
> May be I'm missing some posting but I don't see if
> the patch was tested successfully.
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
Hi Julian
I've had this patch being tested for the last 3-4 days in our regression
rack
and I haven't seen the same issue being reproduced or even a related
crash
or leak in dst.
The original issue was reported only once to us from the regression rack
only
so the exact steps to reproduce is unknown.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH net] tcp: update skb->skb_mstamp more carefully
From: Eric Dumazet @ 2017-09-15 23:16 UTC (permalink / raw)
To: David Miller
Cc: liujian56, edumazet, ycheng, hkchu, netdev, weiyongjun1,
wangkefeng.wang
In-Reply-To: <20170915.143706.992734259378949259.davem@davemloft.net>
On Fri, 2017-09-15 at 14:37 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 13 Sep 2017 20:30:39 -0700
>
> > From: Eric Dumazet <edumazet@googl.com>
> >
> > liujian reported a problem in TCP_USER_TIMEOUT processing with a patch
> > in tcp_probe_timer() :
> > https://www.spinics.net/lists/netdev/msg454496.html
> >
> > After investigations, the root cause of the problem is that we update
> > skb->skb_mstamp of skbs in write queue, even if the attempt to send a
> > clone or copy of it failed. One reason being a routing problem.
> >
> > This patch prevents this, solving liujian issue.
> >
> > It also removes a potential RTT miscalculation, since
> > __tcp_retransmit_skb() is not OR-ing TCP_SKB_CB(skb)->sacked with
> > TCPCB_EVER_RETRANS if a failure happens, but skb->skb_mstamp has
> > been changed.
> >
> > A future ACK would then lead to a very small RTT sample and min_rtt
> > would then be lowered to this too small value.
> >
> > Tested:
> ...
> > Signed-off-by: Eric Dumazet <edumazet@googl.com>
> > Reported-by: liujian <liujian56@huawei.com>
>
> Applied and queued up for -stable, thanks.
Thanks David.
It seems I forgot to move the call to tcp_rate_skb_sent(sk, skb),
as this function depended on skb->skb_mstamp being up to date.
I will send a fix, sorry for not catching this earlier.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: David Miller @ 2017-09-15 23:14 UTC (permalink / raw)
To: rosenp; +Cc: netdev, zajec5, nbd
In-Reply-To: <20170915222218.6613-1-rosenp@gmail.com>
From: Rosen Penev <rosenp@gmail.com>
Date: Fri, 15 Sep 2017 15:22:18 -0700
> On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:
...
> - net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> + net_dev->features &= NETIF_F_GRO;
> net_dev->hw_features = net_dev->features;
This doesn't disable GRO.
^ permalink raw reply
* Re: [PATCH] bgmac: Remove all offloading features, including GRO.
From: Florian Fainelli @ 2017-09-15 22:54 UTC (permalink / raw)
To: Rosen Penev, netdev, zajec5, nbd
In-Reply-To: <20170915222218.6613-1-rosenp@gmail.com>
On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com> wrote:
>On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading
>features give no measurable benefit to speed or latency. Furthermore,
>disabling GRO actually improves iperf performance by a whoppimg 3mbps.
Do you have a way to generate gigabit tests and see what results you are getting? We probably are not going to see a 30% improvement just by extrapolation.
--
Florian
^ permalink raw reply
* Re: selftests/bpf doesn't compile
From: Shuah Khan @ 2017-09-15 22:41 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, Edward Cree
Cc: Shuah Khan, Thomas Meyer, linux-kernel, linux-kselftest,
Networking, Shuah Khan
In-Reply-To: <59BC2069.8050100@iogearbox.net>
On 09/15/2017 12:48 PM, Daniel Borkmann wrote:
> On 09/15/2017 08:23 PM, Daniel Borkmann wrote:
>> On 09/15/2017 08:07 PM, Alexei Starovoitov wrote:
>>> On Fri, Sep 15, 2017 at 05:58:40PM +0100, Edward Cree wrote:
>>>> On 15/09/17 17:02, Alexei Starovoitov wrote:
>>>>> On Thu, Sep 14, 2017 at 09:33:48AM -0600, Shuah Khan wrote:
>>>>>> Is bpf test intended to be run in kselftest run? The clang dependency might
>>>>>> not be met on majority of the systems. Is this a hard dependency??
>>>>> It is a hard dependency and clang should be present on majority of the systems.
>>>> I think this is the wrong approach. Making kselftest hard-require clang doesn't
>>>> mean that the bpf tests will be run more often, it means that the rest of the
>>>> kselftests will be run less often. clang is quite big (when I tried to install
>>>> it on one of my test servers, I didn't have enough disk space & had to go on a
>>>> clear-out of unused packages), and most people aren't interested in the bpf
>>>> subsystem specifically; they would rather be able to skip those tests.
>>>> I feel that as long as they know they are skipping some tests (so e.g. they
>>>> won't consider it a sufficient test of a kselftest refactor), that's fine.
>>>> It's not even as though all of the bpf tests require clang; the (smaller) tests
>>>> written directly in raw eBPF instructions could still be run on such a system.
>>>> So I think we should attempt to run as much as possible but accept that clang
>>>> may not be available and have an option to skip some tests in that case.
>>>
>>> imo the value of selftests/bpf is twofold:
>>> 1. it helps bpf developers avoid regressions
>>> 2. as part of continuous integration it helps to catch bpf regressions
>>> that were somehow caused by changes in other parts of the kernel
>>>
>>> If a developer didn't bother to satisfy all bpf tests dependencies
>>> (which includes clang) and ran all tests before sending a patch,
>>> I don't want to see such patches. It just wastes maintainers time
>>> to review code and spot bugs that could have been caught by tests.
>>> Collectively we invested years of work into these tests and
>>> developers better take advantage of it by running all.
>>
>> +1
>>
>>> If a CI server didn't satisfy all bpf test dependencies,
>>> I don't want such CI setup to be running and reporting results,
>>> since it will give false sense of test coverage.
>>> Test failures due to missing dependencies are hard failures.
>>> We cannot skip them.
>>
>> +1
>
> Btw, on that note, the folks from zero-day bot run the BPF kselftests
> for a while now just fine and they do run them together with clang,
> so they have the full, proper coverage how it should be. It's not
> how it used to be in the early days, you can just go and install
> llvm/clang package on all the major distros today and you get the
> bpf target by default enabled already.
>
>>> I'd like generic XDP tests to be added to selftests/bpf which
>>> would mean that the latest iproute2 will become a hard dependency
>>> and bpf developers and CI host owners would need to upgrade
>>> their iproute2.
>>> The tests either pass or fail. Skipping them due to missing
>>> dependencies is the same as fail and in that sense I don't want
>>> to change selftests/bpf/Makefile to make it skip clang.
>>
>> I fully agree that for the BPF selftests it is very desirable
>> to not only test the verifier with couple of BPF insn snippets,
>> but to actually load and run programs that more closely resemble
>> real world programs. For more complex interactions these snippets
>> are just limited, think of tail calls, testing perf event output
>> helper, etc, which would all require to write these tests with
>> restricted C when we add them (unless we want to make writing
>> these tests a real pain ;) in which case no-one will bother to
>> write tests at all for them). Mid to long term I would definitely
>> like to see more programs in BPF selftests (e.g. moved over from
>> samples/bpf/) to increase the test coverage.
>
As I said in my earlier email:
Unless users choose to install clang, bpf will always fail run without
clang. So clang dependency is an issue for bpf test coverage in general.
That is your choice as to whether you want to increase the scope of
regression test coverage for bpf or not.
I fully understand you have weigh that against ease of writing tests.
We can leave things the way they are since:
- You can't force users to install clang and run bpf test. Users might
opt for letting bpf test fail due to unmet dependency.
- You have reasons to continue use clang and you have been using it for
a longtime.
I will try to see why make ksefltest fails on bpf even with my patch
series that addresses the source directory issue. All other tests build
and run. It is not an issue with bpf specifically, it is something that
has never been tested in this use-case.
thanks,
-- Shuah
^ permalink raw reply
* Re: Use after free in __dst_destroy_metrics_generic
From: Julian Anastasov @ 2017-09-15 22:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, Subash Abhinov Kasiviswanathan,
Linux Kernel Network Developers, Lorenzo Colitti
In-Reply-To: <1505509219.16139.6.camel@edumazet-glaptop3.roam.corp.google.com>
Hello,
On Fri, 15 Sep 2017, Eric Dumazet wrote:
> On Fri, 2017-09-08 at 09:10 -0700, Cong Wang wrote:
> > On Thu, Sep 7, 2017 at 5:52 PM, Subash Abhinov Kasiviswanathan
> > <subashab@codeaurora.org> wrote:
> > > We are seeing a possible use after free in ip6_dst_destroy.
> > >
> > > It appears as if memory of the __DST_METRICS_PTR(old) was freed in some path
> > > and allocated
> > > to ion driver. ion driver has also freed it. Finally the memory is freed by
> > > the
> > > fib gc and crashes since it is already deallocated.
> >
> > Does the attach (compile-only) patch help anything?
> >
> > From my _quick_ glance, it seems we miss the refcnt'ing
> > right in __dst_destroy_metrics_generic().
> >
> > Thanks!
>
>
> Hi Cong
>
> I believe your patch makes a lot of sense, please submit it formally ?
Cong's patch is wrong for few reasons:
- it will stop to kfree non-refcounted metrics
- report was for IPV6 and we set DST_METRICS_REFCOUNTED only
for IPv4, for DST_METRICS_READ_ONLY metrics
- __dst_destroy_metrics_generic is called for val without
DST_METRICS_READ_ONLY flag and such metrics are not with
DST_METRICS_REFCOUNTED flag
- ->cow_metrics and dst_cow_metrics_generic are called with
DST_METRICS_READ_ONLY flag set, there is no chance to write
new value twice, especially to write DST_METRICS_REFCOUNTED flag
and later to see this flag in __dst_destroy_metrics_generic
So, I'm not sure where exactly is the bug with the
metrics.
May be I'm missing some posting but I don't see if
the patch was tested successfully.
Regards
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox