Netdev List
 help / color / mirror / Atom feed
* [PATCH v2 0/4] net: fec: ptp: Add FEC PPS ouput support.
From: Luwei Zhou @ 2014-09-28  4:20 UTC (permalink / raw)
  To: davem, richardcochran
  Cc: netdev, shawn.guo, bhutchings, R49496, b38611, b20596, stephen

These patches use hardeware adjustment to ptp timer and add pps ouput
support.

Change from v1 to v2:
	- Fix the potential 32-bit multiplication overflow issue.
	- Optimize the hareware adjustment code to improve efficiency as Richard suggested
	- Use ptp PTP_PIN_SET/GETFUNC interface to set PPS channel not device tree
	  and add PTP_PF_PPS enumeration
	- Modify comments style
	
Luwei Zhou (4):
  net: fec: ptp: Use the 31-bit ptp timer.
  net: fec: ptp: Use hardware algorithm to adjust PTP counter.
  ptp: Add PTP_PF_PPS enumeration to ptp_pin_function.
  net: fec: ptp: Enalbe PPS ouput based on ptp clock

 drivers/net/ethernet/freescale/fec.h      |  10 +
 drivers/net/ethernet/freescale/fec_main.c |   2 +
 drivers/net/ethernet/freescale/fec_ptp.c  | 293 ++++++++++++++++++++++++++++--
 drivers/ptp/ptp_chardev.c                 |   8 +
 include/linux/ptp_clock_kernel.h          |   2 +
 include/uapi/linux/ptp_clock.h            |   1 +
 6 files changed, 298 insertions(+), 18 deletions(-)

-- 
1.9.1

^ permalink raw reply

* RE: [PATCH v1 0/4] net: fec: Code cleanup
From: fugang.duan @ 2014-09-28  3:39 UTC (permalink / raw)
  To: David Miller; +Cc: netdev@vger.kernel.org, Frank.Li@freescale.com
In-Reply-To: <20140927.185501.2157704066172736196.davem@davemloft.net>

From: David Miller <davem@davemloft.net> Sent: Sunday, September 28, 2014 6:55 AM
>To: Duan Fugang-B38611
>Cc: netdev@vger.kernel.org; Li Frank-B20596
>Subject: Re: [PATCH v1 0/4] net: fec: Code cleanup
>
>From: "fugang.duan@freescale.com" <fugang.duan@freescale.com>
>Date: Sat, 27 Sep 2014 09:51:52 +0000
>
>> From: David Miller <davem@davemloft.net> Sent: Saturday, September 27,
>> 2014 4:06 AM
>>>To: Duan Fugang-B38611
>>>Cc: netdev@vger.kernel.org; Li Frank-B20596
>>>Subject: Re: [PATCH v1 0/4] net: fec: Code cleanup
>>>
>>>From: Fugang Duan <b38611@freescale.com>
>>>Date: Tue, 23 Sep 2014 15:40:54 +0800
>>>
>>>> This patches does several things:
>>>>   - Fixing multiqueue issue.
>>>>   - Removing the unnecessary errata workaround.
>>>>   - Aligning the data buffer dma map/unmap size.
>>>>   - Freeing resource after probe failed.
>>>
>>>Series applied to net-next, thanks.
>>>
>>>Please always explicitly state what tree you expect your patches to be
>>>applied to in the future.
>>
>> No special requirement, just follow the normal rel flow.
>
>That doesn't mean anything to me.
>
>You must state what tree your patches apply properly to, and thus where
>you expect them to be intergrated.

I see. It is better let them enter to 3.17 (rc7).

Thanks,
Andy 

^ permalink raw reply

* RE: [PATCH] net: fec: remove memory copy for rx path
From: fugang.duan @ 2014-09-28  3:30 UTC (permalink / raw)
  To: David Miller, romieu@fr.zoreil.com
  Cc: Frank.Li@freescale.com, netdev@vger.kernel.org,
	shawn.guo@linaro.org, bhutchings@solarflare.com
In-Reply-To: <20140927.185600.1057301078747765304.davem@davemloft.net>

From: David Miller <davem@davemloft.net> Sent: Sunday, September 28, 2014 6:56 AM
>To: romieu@fr.zoreil.com
>Cc: Duan Fugang-B38611; Li Frank-B20596; netdev@vger.kernel.org;
>shawn.guo@linaro.org; bhutchings@solarflare.com
>Subject: Re: [PATCH] net: fec: remove memory copy for rx path
>
>From: Francois Romieu <romieu@fr.zoreil.com>
>Date: Sat, 27 Sep 2014 13:39:37 +0200
>
>> fugang.duan@freescale.com <fugang.duan@freescale.com> :
>>> From: David Miller <davem@davemloft.net> Sent: Saturday, September
>>> 27, 2014 5:07 AM
>> [...]
>>> >We call this rx_copybreak, and there are many drivers you can look
>>> >at to see how this works.
>>> >
>>> >Thanks.
>>>
>>> Great suggestion. I will change it for next version.
>>
>> Be aware that copybreak module option should be considered legacy.
>>
>> There is an ethtool api for it.
>
>Right.

Got it, thanks David and romieu. 

Regards,
Andy

^ permalink raw reply

* SKB Documentation Ideas and How to Write Docs for Kernel
From: nick @ 2014-09-28  2:21 UTC (permalink / raw)
  To: davem
  Cc: edumazet, mst, therbert, willemb, fw, Paul.Durrant, tgraf,
	vyasevic, herbert, JBeulich, mszeredi, netdev, linux-kernel

Hello to the maintainers of the file,skb.c ,
I am wondering about making it easier for new developers including myself by writing a file in the docs on networking
related to skb_buff and it's various methods of use to developing in the kernel's various networking subsystems. If one of the maintainers would like to help give me feedback on my writing of these docs this would be helpful and make it 
easier for everyone as from my experience skb_buff is one of the most asked about things in the networking lists and
would allow people more thing for other work, including the maintainers of lots of the networking code. I understand
you guys are very busy so feel free to respond whenever you get free time during the next few weeks.
Thanks Nick 

^ permalink raw reply

* Re: [PATCH net-next] mlx4: optimize xmit path
From: Hannes Frederic Sowa @ 2014-09-28  0:22 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Or Gerlitz, Alexei Starovoitov, David S. Miller,
	Jesper Dangaard Brouer, Eric Dumazet, John Fastabend,
	Linux Netdev List, Amir Vadai, Or Gerlitz
In-Reply-To: <1411862731.15768.63.camel@edumazet-glaptop2.roam.corp.google.com>

On Sun, Sep 28, 2014, at 02:05, Eric Dumazet wrote:
> On Sun, 2014-09-28 at 01:44 +0200, Hannes Frederic Sowa wrote:
> > Hi Eric,
> > 
> > On Sun, Sep 28, 2014, at 00:56, Eric Dumazet wrote:
> > > -       ring->cons += txbbs_skipped;
> > > +
> > > +       /* we want to dirty this cache line once */
> > > +       ACCESS_ONCE(ring->last_nr_txbb) = last_nr_txbb;
> > > +       ACCESS_ONCE(ring->cons) = ring_cons + txbbs_skipped;
> > > +
> > 
> > Impressive work!
> > 
> > I wonder if another macro might be useful for those kind of
> > dereferences, because ACCESS_ONCE is associated with correctness in my
> > mind and those usages only try to optimize access patterns.
> > Does OPTIMIZER_HIDE_VAR generate the same code?
> 
> 
> If we have 
> 
> ring->cons += txbbs_skipped;
> 
> Then compiler might issue a RMW instruction.
> 
> And this is bad in this case.
> 
> I really want to _write_ into this location, and its fast because I
> already have in ring_cons the content I fetched maybe hundred of
> nanoseconds before, or even thousand of nanoseconds before.
> 
> ACCESS_ONCE(XXXX) = y
> 
> Is not only for correctness.
> 
> It exactly documents the fact that we want to perform a single write.
> 
> I believe it is time that people understand how useful is this helper
> (Less than 700 occurrences in the whole kernel today, not including
> Documentation/*)

Understood, thanks.

For me ACCESS_ONCE was something which slowed down code till today. Also
I have the feeling that instruction scheduling in the compiler could do
a better job in some places...

Now I wonder if it is worth it playing around with the restrict keyword
and strict-aliasing in networking. ;)

Bye,
Hannes

^ permalink raw reply

* Re: [PATCH net-next] mlx4: optimize xmit path
From: Eric Dumazet @ 2014-09-28  0:05 UTC (permalink / raw)
  To: Hannes Frederic Sowa
  Cc: Or Gerlitz, Alexei Starovoitov, David S. Miller,
	Jesper Dangaard Brouer, Eric Dumazet, John Fastabend,
	Linux Netdev List, Amir Vadai, Or Gerlitz
In-Reply-To: <1411861467.374982.172498061.37EB43B1@webmail.messagingengine.com>

On Sun, 2014-09-28 at 01:44 +0200, Hannes Frederic Sowa wrote:
> Hi Eric,
> 
> On Sun, Sep 28, 2014, at 00:56, Eric Dumazet wrote:
> > -       ring->cons += txbbs_skipped;
> > +
> > +       /* we want to dirty this cache line once */
> > +       ACCESS_ONCE(ring->last_nr_txbb) = last_nr_txbb;
> > +       ACCESS_ONCE(ring->cons) = ring_cons + txbbs_skipped;
> > +
> 
> Impressive work!
> 
> I wonder if another macro might be useful for those kind of
> dereferences, because ACCESS_ONCE is associated with correctness in my
> mind and those usages only try to optimize access patterns.
> Does OPTIMIZER_HIDE_VAR generate the same code?


If we have 

ring->cons += txbbs_skipped;

Then compiler might issue a RMW instruction.

And this is bad in this case.

I really want to _write_ into this location, and its fast because I
already have in ring_cons the content I fetched maybe hundred of
nanoseconds before, or even thousand of nanoseconds before.

ACCESS_ONCE(XXXX) = y

Is not only for correctness.

It exactly documents the fact that we want to perform a single write.

I believe it is time that people understand how useful is this helper
(Less than 700 occurrences in the whole kernel today, not including
Documentation/*)

^ permalink raw reply

* Re: [PATCH net-next] mlx4: optimize xmit path
From: Hannes Frederic Sowa @ 2014-09-27 23:44 UTC (permalink / raw)
  To: Eric Dumazet, Or Gerlitz
  Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
	Eric Dumazet, John Fastabend, Linux Netdev List, Amir Vadai,
	Or Gerlitz
In-Reply-To: <1411858593.15768.51.camel@edumazet-glaptop2.roam.corp.google.com>

Hi Eric,

On Sun, Sep 28, 2014, at 00:56, Eric Dumazet wrote:
> -       ring->cons += txbbs_skipped;
> +
> +       /* we want to dirty this cache line once */
> +       ACCESS_ONCE(ring->last_nr_txbb) = last_nr_txbb;
> +       ACCESS_ONCE(ring->cons) = ring_cons + txbbs_skipped;
> +

Impressive work!

I wonder if another macro might be useful for those kind of
dereferences, because ACCESS_ONCE is associated with correctness in my
mind and those usages only try to optimize access patterns.
Does OPTIMIZER_HIDE_VAR generate the same code?

Bye,
Hannes

^ permalink raw reply

* Re: [PATCH v2 net-next 0/9] ipv6: fib6: socket dst_entry improvments and cleanups
From: Hannes Frederic Sowa @ 2014-09-27 23:32 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, eric.dumazet, hideaki, vyasevich, nicolas.dichtel, kafai
In-Reply-To: <20140926.124643.202150841147181272.davem@davemloft.net>

On Fri, Sep 26, 2014, at 18:46, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Fri, 26 Sep 2014 09:58:43 +0200
> 
> > On Fri, Sep 26, 2014, at 06:28, David Miller wrote:
> >> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >> Date: Sun, 21 Sep 2014 16:11:44 +0200
> >> 
> >> > Eric Dumazet noticed that rt6_nodes wich are neither RTF_NONEXTHOP nor
> >> > RTF_GATEWAY but DST_HOST ones cause major routing lookup churn because
> >> > their rt6_genid is never renewed, thus ip6_dst_check always considers
> >> > them outdated. This is a major problem, because these kind of routes
> >> > are normally used to in input handling.
> >> 
> >> This series is a disappointment for me from the perspective of the
> >> fact that we have a regression in mainline and this is too complex
> >> of a set of changes for there.
> >> 
> >> If we relookup the thing every TCP input packet, we might as well
> >> not do the input route caching in the socket.
> > 
> > I can understand.
> > 
> > Toss this series, I'll try to do better tomorrow and send changes for
> > net and submit net-next cleanups when your queue is a bit smaller.
> 
> BTW, don't get me wrong, I like the new code and for 'net-next' it's
> good.

I didn't. ;)

> But for 'net' we have to come up with something simpler meanwhile.

Sure, I just posted one small patch to address the problem, cleanups and
smaller performance optimizations come in later after you did a merge.

Thanks,
Hannes

^ permalink raw reply

* bna alloc_pages() order 2 failure in bnad.c bnad_rxq_refill_page()
From: Eric Wheeler @ 2014-09-27 23:44 UTC (permalink / raw)
  To: netdev; +Cc: rmody

Hello all,

We're using the 10gbe bna card and sometimes we get pages and pages of 
alloc_pages() failure backtraces like below.  (The maintainer 
rmody@brocade.com does not appear to have an active email at brocade, but 
cc'ing again just in case.)

It looks like bnad_rxq_refill_page() in bnad.c is allocating for the 
receive queue but fails.  We've already tried bumping vm.min_free_kbytes 
and vm.zone_reclaim_mode but it doesn't appear to help.

Suggestions?

Would it be appropriate to convert alloc_pages() to a mempool 
implementation?

-Eric

[135367.300669] swapper/5: page allocation failure: order:2, mode:0x4020
[135367.300903] CPU: 5 PID: 0 Comm: swapper/5 Tainted: GF          O 3.14.18 #1
[135367.301137] Hardware name: Supermicro X9SCL/X9SCM/X9SCL/X9SCM, BIOS 2.0c 10/17/2013
[135367.301557]  0000000000000002 ffff88082fd43a78 ffffffff8159e704 0000000000000010
[135367.301986]  0000000000004020 ffff88082fd43b08 ffffffff8113b17b 0000000200000000
[135367.302417]  0000000000000000 00000000fffffffc 0000000000000004 0000003000000000
[135367.302858] Call Trace:
[135367.303077]  <IRQ> [<ffffffff8159e704>] dump_stack+0x49/0x5d
[135367.303310]  [<ffffffff8113b17b>] warn_alloc_failed+0xeb/0x150
[135367.303536]  [<ffffffff8113e2f0>] __alloc_pages_slowpath+0x4a0/0x7e0
[135367.303769]  [<ffffffff8113e8e2>] __alloc_pages_nodemask+0x2b2/0x2c0
[135367.303996]  [<ffffffff81181232>] alloc_pages_current+0xb2/0x170 
[135367.304226]  [<ffffffffa0239cb4>] bnad_rxq_refill_page+0x154/0x1e0 [bna] <---
[135367.304459]  [<ffffffffa023c282>] bnad_cq_process+0x462/0x840 [bna]
[135367.304921]  [<ffffffffa023c6af>] bnad_napi_poll_rx+0x4f/0xc0 [bna]
[135367.305144]  [<ffffffff814dcf4c>] net_rx_action+0xfc/0x280
[135367.305364]  [<ffffffff8105ba83>] __do_softirq+0xf3/0x2c0
[135367.305589]  [<ffffffff8105bd5d>] irq_exit+0xbd/0xd0
[135367.305811]  [<ffffffff815ae697>] do_IRQ+0x67/0x110
[135367.306036]  [<ffffffff815a39ad>] common_interrupt+0x6d/0x6d
[135367.306261]  <EOI> [<ffffffff81486275>] ? cpuidle_enter_state+0x55/0xd0
[135367.306501]  [<ffffffff8148626b>] ? cpuidle_enter_state+0x4b/0xd0
[135367.306719]  [<ffffffff814863b7>] cpuidle_idle_call+0xc7/0x160
[135367.306938]  [<ffffffff8100d73e>] arch_cpu_idle+0xe/0x30
[135367.307156]  [<ffffffff810b187e>] cpu_idle_loop+0x9e/0x250
[135367.307376]  [<ffffffff810b1aa0>] cpu_startup_entry+0x70/0x80
[135367.307598]  [<ffffffff810350d2>] start_secondary+0xd2/0xe0



--
Eric Wheeler, President           eWheeler, Inc. dba Global Linux Security
888-LINUX26 (888-546-8926)        Fax: 503-716-3878           PO Box 25107
www.GlobalLinuxSecurity.pro       Linux since 1996!     Portland, OR 97298

^ permalink raw reply

* [PATCH net-next] net: cleanup and document skb fclone layout
From: Eric Dumazet @ 2014-09-27 23:25 UTC (permalink / raw)
  To: David Miller; +Cc: netdev

From: Eric Dumazet <edumazet@google.com>

Lets use a proper structure to clearly document and implement
skb fast clones.

Then, we might experiment more easily alternative layouts.

This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h |   25 ++++++++++++++++++++++
 net/core/skbuff.c      |   44 +++++++++++++++++++--------------------
 net/ipv4/tcp_output.c  |    5 ----
 net/xfrm/xfrm_policy.c |    4 ---
 4 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8eaa62400fca..ed5fc2faa074 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -775,6 +775,31 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
 				     int *errcode,
 				     gfp_t gfp_mask);
 
+/* Layout of fast clones : [skb1][skb2][fclone_ref] */
+struct sk_buff_fclones {
+	struct sk_buff	skb1;
+
+	struct sk_buff	skb2;
+
+	atomic_t	fclone_ref;
+};
+
+/**
+ *	skb_fclone_busy - check if fclone is busy
+ *	@skb: buffer
+ *
+ * Returns true is skb is a fast clone, and its clone is not freed.
+ */
+static inline bool skb_fclone_busy(const struct sk_buff *skb)
+{
+	const struct sk_buff_fclones *fclones;
+
+	fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+	return skb->fclone == SKB_FCLONE_ORIG &&
+	       fclones->skb2.fclone == SKB_FCLONE_CLONE;
+}
+
 static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
 					       gfp_t priority)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d4fdc649112c..460f5f4f12a9 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -257,16 +257,18 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	kmemcheck_annotate_variable(shinfo->destructor_arg);
 
 	if (flags & SKB_ALLOC_FCLONE) {
-		struct sk_buff *child = skb + 1;
-		atomic_t *fclone_ref = (atomic_t *) (child + 1);
+		struct sk_buff_fclones *fclones;
 
-		kmemcheck_annotate_bitfield(child, flags1);
-		kmemcheck_annotate_bitfield(child, flags2);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+		kmemcheck_annotate_bitfield(&fclones->skb2, flags1);
+		kmemcheck_annotate_bitfield(&fclones->skb2, flags2);
 		skb->fclone = SKB_FCLONE_ORIG;
-		atomic_set(fclone_ref, 1);
 
-		child->fclone = SKB_FCLONE_UNAVAILABLE;
-		child->pfmemalloc = pfmemalloc;
+		atomic_set(&fclones->fclone_ref, 1);
+
+		fclones->skb2.fclone = SKB_FCLONE_UNAVAILABLE;
+		fclones->skb2.pfmemalloc = pfmemalloc;
 	}
 out:
 	return skb;
@@ -525,8 +527,7 @@ static void skb_release_data(struct sk_buff *skb)
  */
 static void kfree_skbmem(struct sk_buff *skb)
 {
-	struct sk_buff *other;
-	atomic_t *fclone_ref;
+	struct sk_buff_fclones *fclones;
 
 	switch (skb->fclone) {
 	case SKB_FCLONE_UNAVAILABLE:
@@ -534,22 +535,21 @@ static void kfree_skbmem(struct sk_buff *skb)
 		break;
 
 	case SKB_FCLONE_ORIG:
-		fclone_ref = (atomic_t *) (skb + 2);
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, skb);
+		fclones = container_of(skb, struct sk_buff_fclones, skb1);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 
 	case SKB_FCLONE_CLONE:
-		fclone_ref = (atomic_t *) (skb + 1);
-		other = skb - 1;
+		fclones = container_of(skb, struct sk_buff_fclones, skb2);
 
 		/* The clone portion is available for
 		 * fast-cloning again.
 		 */
 		skb->fclone = SKB_FCLONE_UNAVAILABLE;
 
-		if (atomic_dec_and_test(fclone_ref))
-			kmem_cache_free(skbuff_fclone_cache, other);
+		if (atomic_dec_and_test(&fclones->fclone_ref))
+			kmem_cache_free(skbuff_fclone_cache, fclones);
 		break;
 	}
 }
@@ -856,17 +856,18 @@ EXPORT_SYMBOL_GPL(skb_copy_ubufs);
 
 struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 {
-	struct sk_buff *n;
+	struct sk_buff_fclones *fclones = container_of(skb,
+						       struct sk_buff_fclones,
+						       skb1);
+	struct sk_buff *n = &fclones->skb2;
 
 	if (skb_orphan_frags(skb, gfp_mask))
 		return NULL;
 
-	n = skb + 1;
 	if (skb->fclone == SKB_FCLONE_ORIG &&
 	    n->fclone == SKB_FCLONE_UNAVAILABLE) {
-		atomic_t *fclone_ref = (atomic_t *) (n + 1);
 		n->fclone = SKB_FCLONE_CLONE;
-		atomic_inc(fclone_ref);
+		atomic_inc(&fclones->fclone_ref);
 	} else {
 		if (skb_pfmemalloc(skb))
 			gfp_mask |= __GFP_MEMALLOC;
@@ -3238,8 +3239,7 @@ void __init skb_init(void)
 					      SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 					      NULL);
 	skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
-						(2*sizeof(struct sk_buff)) +
-						sizeof(atomic_t),
+						sizeof(struct sk_buff_fclones),
 						0,
 						SLAB_HWCACHE_ALIGN|SLAB_PANIC,
 						NULL);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f173b1c4f815..fd72d381d924 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2091,10 +2091,7 @@ bool tcp_schedule_loss_probe(struct sock *sk)
 static bool skb_still_in_host_queue(const struct sock *sk,
 				    const struct sk_buff *skb)
 {
-	const struct sk_buff *fclone = skb + 1;
-
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		NET_INC_STATS_BH(sock_net(sk),
 				 LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
 		return true;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index fdde51f4271a..b31cb8354c97 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1844,10 +1844,8 @@ static int xdst_queue_output(struct sock *sk, struct sk_buff *skb)
 	struct xfrm_dst *xdst = (struct xfrm_dst *) dst;
 	struct xfrm_policy *pol = xdst->pols[0];
 	struct xfrm_policy_queue *pq = &pol->polq;
-	const struct sk_buff *fclone = skb + 1;
 
-	if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
-		     fclone->fclone == SKB_FCLONE_CLONE)) {
+	if (unlikely(skb_fclone_busy(skb))) {
 		kfree_skb(skb);
 		return 0;
 	}

^ permalink raw reply related

* [PATCH net-next] mlx4: optimize xmit path
From: Eric Dumazet @ 2014-09-27 22:56 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
	Eric Dumazet, John Fastabend, Linux Netdev List, Amir Vadai,
	Or Gerlitz
In-Reply-To: <1411853441.15768.13.camel@edumazet-glaptop2.roam.corp.google.com>

From: Eric Dumazet <edumazet@google.com>

First I implemented skb->xmit_more support, and pktgen throughput
went from ~5Mpps to ~10Mpps.

Then, looking closely at this driver I found false sharing problems that
should be addressed by this patch, as my pktgen now reaches 14.7 Mpps
on a single TX queue, with a burst factor of 8.

So this patch in a whole permits to improve raw performance on a single
TX queue from about 5 Mpps to 14.7 Mpps.

Note that if packets are below the inline_thold threshold (104 bytes),
driver copies packets content into tx descriptor, and throughput
is lowered to ~7 Mpps :
->	We might reconsider inlining strategy in a followup patch.

I could split this patch into multiple components, but I prefer not
spend days on this work.

Lets instead list all changes I did :

1) align struct mlx4_en_tx_info to a cache line

2) add frag0_dma/frag0_byte_count into mlx4_en_tx_info to avoid a cache
   line miss in TX completion for frames having one dma element.
   (We avoid reading back the tx descriptor)
   Note this could be extended to 2/3 dma elements later,
   as we have free room in mlx4_en_tx_info

3) reorganize struct mlx4_en_tx_ring to have 
   3.1 - One cache line containing last_nr_txbb & cons,
     used by tx completion.
   3.2 - One cache line containing fields dirtied by mlx4_en_xmit()  
   3.3 - Following part is read mostly and shared by cpus.

4) struct mlx4_bf @offset field reduced to unsigned int to save space
   so that the 3.2 part is only 64 bytes, or one cache line on x86.

5) doorbell_qpn is stored in the cpu_to_be32() way to avoid bswap() in
   fast path

6) mdev->mr.key stored in ring->mr_key to also avoid bswap() and access
   to cold cache line.

7) mlx4_en_free_tx_desc() no longer accesses skb_shinfo(). We use a new
   nr_frags fields in mlx4_en_tx_info to avoid 2 or 3 cache misses.

8) mlx4_en_free_tx_desc() uses a prefetchw(&skb->users) to speed up
   consume_skb()

9) mlx4_en_process_tx_cq() carefully fetches and writes
   ring->last_nr_txbb & ring->cons only one time to avoid false sharing

10) mlx4_en_xmit() reads ring->cons once, and ahead of time to avoid
    stalls.

11) prefetchw(&ring->tx_queue->dql) to speed up BQL update

12) properly clears tx_info->ts_requested :
    This field was never cleared.

13) Support skb->xmit_more to avoid the expensive doorbell

14) reorganize code to call is_inline() once, so compiler can inline it.

15) rename @i variable to @i_frag to avoid confusion, as the
    "goto tx_drop_unmap;" relied on this @i variable.
 
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c   |  312 ++++++++++-------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h |   89 ++--
 include/linux/mlx4/device.h                  |    2 
 3 files changed, 241 insertions(+), 162 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index c44f4237b9be..fa29d53860a6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -191,12 +191,12 @@ int mlx4_en_activate_tx_ring(struct mlx4_en_priv *priv,
 	ring->prod = 0;
 	ring->cons = 0xffffffff;
 	ring->last_nr_txbb = 1;
-	ring->poll_cnt = 0;
 	memset(ring->tx_info, 0, ring->size * sizeof(struct mlx4_en_tx_info));
 	memset(ring->buf, 0, ring->buf_size);
 
 	ring->qp_state = MLX4_QP_STATE_RST;
-	ring->doorbell_qpn = ring->qp.qpn << 8;
+	ring->doorbell_qpn = cpu_to_be32(ring->qp.qpn << 8);
+	ring->mr_key = cpu_to_be32(mdev->mr.key);
 
 	mlx4_en_fill_qp_context(priv, ring->size, ring->stride, 1, 0, ring->qpn,
 				ring->cqn, user_prio, &ring->context);
@@ -259,38 +259,45 @@ static u32 mlx4_en_free_tx_desc(struct mlx4_en_priv *priv,
 				struct mlx4_en_tx_ring *ring,
 				int index, u8 owner, u64 timestamp)
 {
-	struct mlx4_en_dev *mdev = priv->mdev;
 	struct mlx4_en_tx_info *tx_info = &ring->tx_info[index];
 	struct mlx4_en_tx_desc *tx_desc = ring->buf + index * TXBB_SIZE;
 	struct mlx4_wqe_data_seg *data = (void *) tx_desc + tx_info->data_offset;
-	struct sk_buff *skb = tx_info->skb;
-	struct skb_frag_struct *frag;
 	void *end = ring->buf + ring->buf_size;
-	int frags = skb_shinfo(skb)->nr_frags;
+	struct sk_buff *skb = tx_info->skb;
+	int nr_frags = tx_info->nr_frags;
 	int i;
-	struct skb_shared_hwtstamps hwts;
 
-	if (timestamp) {
-		mlx4_en_fill_hwtstamps(mdev, &hwts, timestamp);
+	/* We do not touch skb here, so prefetch skb->users location
+	 * to speedup consume_skb()
+	 */
+	prefetchw(&skb->users);
+
+	if (unlikely(timestamp)) {
+		struct skb_shared_hwtstamps hwts;
+
+		mlx4_en_fill_hwtstamps(priv->mdev, &hwts, timestamp);
 		skb_tstamp_tx(skb, &hwts);
 	}
 
 	/* Optimize the common case when there are no wraparounds */
 	if (likely((void *) tx_desc + tx_info->nr_txbb * TXBB_SIZE <= end)) {
 		if (!tx_info->inl) {
-			if (tx_info->linear) {
+			if (tx_info->linear)
 				dma_unmap_single(priv->ddev,
-					(dma_addr_t) be64_to_cpu(data->addr),
-					 be32_to_cpu(data->byte_count),
-					 PCI_DMA_TODEVICE);
-				++data;
-			}
-
-			for (i = 0; i < frags; i++) {
-				frag = &skb_shinfo(skb)->frags[i];
+						tx_info->frag0_dma,
+						tx_info->frag0_byte_count,
+						PCI_DMA_TODEVICE);
+			else
+				dma_unmap_page(priv->ddev,
+					       tx_info->frag0_dma,
+					       tx_info->frag0_byte_count,
+					       PCI_DMA_TODEVICE);
+			for (i = 1; i < nr_frags; i++) {
+				data++;
 				dma_unmap_page(priv->ddev,
-					(dma_addr_t) be64_to_cpu(data[i].addr),
-					skb_frag_size(frag), PCI_DMA_TODEVICE);
+					(dma_addr_t)be64_to_cpu(data->addr),
+					be32_to_cpu(data->byte_count),
+					PCI_DMA_TODEVICE);
 			}
 		}
 	} else {
@@ -299,22 +306,24 @@ static u32 mlx4_en_free_tx_desc(struct mlx4_en_priv *priv,
 				data = ring->buf + ((void *)data - end);
 			}
 
-			if (tx_info->linear) {
+			if (tx_info->linear)
 				dma_unmap_single(priv->ddev,
-					(dma_addr_t) be64_to_cpu(data->addr),
-					 be32_to_cpu(data->byte_count),
-					 PCI_DMA_TODEVICE);
-				++data;
-			}
-
-			for (i = 0; i < frags; i++) {
+						tx_info->frag0_dma,
+						tx_info->frag0_byte_count,
+						PCI_DMA_TODEVICE);
+			else
+				dma_unmap_page(priv->ddev,
+					       tx_info->frag0_dma,
+					       tx_info->frag0_byte_count,
+					       PCI_DMA_TODEVICE);
+			for (i = 1; i < nr_frags; i++) {
 				/* Check for wraparound before unmapping */
 				if ((void *) data >= end)
 					data = ring->buf;
-				frag = &skb_shinfo(skb)->frags[i];
 				dma_unmap_page(priv->ddev,
-					(dma_addr_t) be64_to_cpu(data->addr),
-					 skb_frag_size(frag), PCI_DMA_TODEVICE);
+					(dma_addr_t)be64_to_cpu(data->addr),
+					be32_to_cpu(data->byte_count),
+					PCI_DMA_TODEVICE);
 				++data;
 			}
 		}
@@ -377,13 +386,18 @@ static bool mlx4_en_process_tx_cq(struct net_device *dev,
 	u64 timestamp = 0;
 	int done = 0;
 	int budget = priv->tx_work_limit;
+	u32 last_nr_txbb;
+	u32 ring_cons;
 
 	if (!priv->port_up)
 		return true;
 
+	prefetchw(&ring->tx_queue->dql.limit);
 	index = cons_index & size_mask;
 	cqe = mlx4_en_get_cqe(buf, index, priv->cqe_size) + factor;
-	ring_index = ring->cons & size_mask;
+	last_nr_txbb = ACCESS_ONCE(ring->last_nr_txbb);
+	ring_cons = ACCESS_ONCE(ring->cons);
+	ring_index = ring_cons & size_mask;
 	stamp_index = ring_index;
 
 	/* Process all completed CQEs */
@@ -408,19 +422,19 @@ static bool mlx4_en_process_tx_cq(struct net_device *dev,
 		new_index = be16_to_cpu(cqe->wqe_index) & size_mask;
 
 		do {
-			txbbs_skipped += ring->last_nr_txbb;
-			ring_index = (ring_index + ring->last_nr_txbb) & size_mask;
+			txbbs_skipped += last_nr_txbb;
+			ring_index = (ring_index + last_nr_txbb) & size_mask;
 			if (ring->tx_info[ring_index].ts_requested)
 				timestamp = mlx4_en_get_cqe_ts(cqe);
 
 			/* free next descriptor */
-			ring->last_nr_txbb = mlx4_en_free_tx_desc(
+			last_nr_txbb = mlx4_en_free_tx_desc(
 					priv, ring, ring_index,
-					!!((ring->cons + txbbs_skipped) &
+					!!((ring_cons + txbbs_skipped) &
 					ring->size), timestamp);
 
 			mlx4_en_stamp_wqe(priv, ring, stamp_index,
-					  !!((ring->cons + txbbs_stamp) &
+					  !!((ring_cons + txbbs_stamp) &
 						ring->size));
 			stamp_index = ring_index;
 			txbbs_stamp = txbbs_skipped;
@@ -441,7 +455,11 @@ static bool mlx4_en_process_tx_cq(struct net_device *dev,
 	mcq->cons_index = cons_index;
 	mlx4_cq_set_ci(mcq);
 	wmb();
-	ring->cons += txbbs_skipped;
+
+	/* we want to dirty this cache line once */
+	ACCESS_ONCE(ring->last_nr_txbb) = last_nr_txbb;
+	ACCESS_ONCE(ring->cons) = ring_cons + txbbs_skipped;
+
 	netdev_tx_completed_queue(ring->tx_queue, packets, bytes);
 
 	/*
@@ -512,30 +530,35 @@ static struct mlx4_en_tx_desc *mlx4_en_bounce_to_desc(struct mlx4_en_priv *priv,
 	return ring->buf + index * TXBB_SIZE;
 }
 
-static int is_inline(int inline_thold, struct sk_buff *skb, void **pfrag)
+/* Decide if skb can be inlined in tx descriptor to avoid dma mapping
+ *
+ * It seems strange we do not simply use skb_copy_bits().
+ * This would allow to inline all skbs iff skb->len <= inline_thold
+ *
+ * Note that caller already checked skb was not a gso packet
+ */
+static bool is_inline(int inline_thold, const struct sk_buff *skb,
+		      const struct skb_shared_info *shinfo,
+		      void **pfrag)
 {
 	void *ptr;
 
-	if (inline_thold && !skb_is_gso(skb) && skb->len <= inline_thold) {
-		if (skb_shinfo(skb)->nr_frags == 1) {
-			ptr = skb_frag_address_safe(&skb_shinfo(skb)->frags[0]);
-			if (unlikely(!ptr))
-				return 0;
-
-			if (pfrag)
-				*pfrag = ptr;
+	if (skb->len > inline_thold || !inline_thold)
+		return false;
 
-			return 1;
-		} else if (unlikely(skb_shinfo(skb)->nr_frags))
-			return 0;
-		else
-			return 1;
+	if (shinfo->nr_frags == 1) {
+		ptr = skb_frag_address_safe(&shinfo->frags[0]);
+		if (unlikely(!ptr))
+			return false;
+		*pfrag = ptr;
+		return true;
 	}
-
-	return 0;
+	if (shinfo->nr_frags)
+		return false;
+	return true;
 }
 
-static int inline_size(struct sk_buff *skb)
+static int inline_size(const struct sk_buff *skb)
 {
 	if (skb->len + CTRL_SIZE + sizeof(struct mlx4_wqe_inline_seg)
 	    <= MLX4_INLINE_ALIGN)
@@ -546,18 +569,23 @@ static int inline_size(struct sk_buff *skb)
 			     sizeof(struct mlx4_wqe_inline_seg), 16);
 }
 
-static int get_real_size(struct sk_buff *skb, struct net_device *dev,
-			 int *lso_header_size)
+static int get_real_size(const struct sk_buff *skb,
+			 const struct skb_shared_info *shinfo,
+			 struct net_device *dev,
+			 int *lso_header_size,
+			 bool *inline_ok,
+			 void **pfrag)
 {
 	struct mlx4_en_priv *priv = netdev_priv(dev);
 	int real_size;
 
-	if (skb_is_gso(skb)) {
+	if (shinfo->gso_size) {
+		*inline_ok = false;
 		if (skb->encapsulation)
 			*lso_header_size = (skb_inner_transport_header(skb) - skb->data) + inner_tcp_hdrlen(skb);
 		else
 			*lso_header_size = skb_transport_offset(skb) + tcp_hdrlen(skb);
-		real_size = CTRL_SIZE + skb_shinfo(skb)->nr_frags * DS_SIZE +
+		real_size = CTRL_SIZE + shinfo->nr_frags * DS_SIZE +
 			ALIGN(*lso_header_size + 4, DS_SIZE);
 		if (unlikely(*lso_header_size != skb_headlen(skb))) {
 			/* We add a segment for the skb linear buffer only if
@@ -572,17 +600,24 @@ static int get_real_size(struct sk_buff *skb, struct net_device *dev,
 		}
 	} else {
 		*lso_header_size = 0;
-		if (!is_inline(priv->prof->inline_thold, skb, NULL))
-			real_size = CTRL_SIZE + (skb_shinfo(skb)->nr_frags + 1) * DS_SIZE;
-		else
+		*inline_ok = is_inline(priv->prof->inline_thold, skb,
+				       shinfo, pfrag);
+
+		if (*inline_ok)
 			real_size = inline_size(skb);
+		else
+			real_size = CTRL_SIZE +
+				    (shinfo->nr_frags + 1) * DS_SIZE;
 	}
 
 	return real_size;
 }
 
-static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, struct sk_buff *skb,
-			     int real_size, u16 *vlan_tag, int tx_ind, void *fragptr)
+static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc,
+			     const struct sk_buff *skb,
+			     const struct skb_shared_info *shinfo,
+			     int real_size, u16 *vlan_tag,
+			     int tx_ind, void *fragptr)
 {
 	struct mlx4_wqe_inline_seg *inl = &tx_desc->inl;
 	int spc = MLX4_INLINE_ALIGN - CTRL_SIZE - sizeof *inl;
@@ -596,9 +631,9 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, struct sk_buff *sk
 			       MIN_PKT_LEN - skb->len);
 		}
 		skb_copy_from_linear_data(skb, inl + 1, skb_headlen(skb));
-		if (skb_shinfo(skb)->nr_frags)
+		if (shinfo->nr_frags)
 			memcpy(((void *)(inl + 1)) + skb_headlen(skb), fragptr,
-			       skb_frag_size(&skb_shinfo(skb)->frags[0]));
+			       skb_frag_size(&shinfo->frags[0]));
 
 	} else {
 		inl->byte_count = cpu_to_be32(1 << 31 | spc);
@@ -616,9 +651,10 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, struct sk_buff *sk
 			inl = (void *) (inl + 1) + spc;
 			skb_copy_from_linear_data_offset(skb, spc, inl + 1,
 					skb_headlen(skb) - spc);
-			if (skb_shinfo(skb)->nr_frags)
+			if (shinfo->nr_frags)
 				memcpy(((void *)(inl + 1)) + skb_headlen(skb) - spc,
-					fragptr, skb_frag_size(&skb_shinfo(skb)->frags[0]));
+				       fragptr,
+				       skb_frag_size(&shinfo->frags[0]));
 		}
 
 		wmb();
@@ -642,7 +678,8 @@ u16 mlx4_en_select_queue(struct net_device *dev, struct sk_buff *skb,
 	return fallback(dev, skb) % rings_p_up + up * rings_p_up;
 }
 
-static void mlx4_bf_copy(void __iomem *dst, unsigned long *src, unsigned bytecnt)
+static void mlx4_bf_copy(void __iomem *dst, const void *src,
+			 unsigned int bytecnt)
 {
 	__iowrite64_copy(dst, src, bytecnt / 8);
 }
@@ -663,15 +700,26 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 index, bf_index;
 	__be32 op_own;
 	u16 vlan_tag = 0;
-	int i;
+	int i_frag;
 	int lso_header_size;
-	void *fragptr;
+	void *fragptr = NULL;
 	bool bounce = false;
+	bool send_doorbell;
+	u32 ring_cons;
+	struct skb_shared_info *shinfo = skb_shinfo(skb);
+	bool inline_ok;
 
 	if (!priv->port_up)
 		goto tx_drop;
 
-	real_size = get_real_size(skb, dev, &lso_header_size);
+	tx_ind = skb_get_queue_mapping(skb);
+	ring = priv->tx_ring[tx_ind];
+
+	/* fetch ring->cons far ahead before needing it to avoid stall */
+	ring_cons = ACCESS_ONCE(ring->cons);
+
+	real_size = get_real_size(skb, shinfo, dev, &lso_header_size,
+				  &inline_ok, &fragptr);
 	if (unlikely(!real_size))
 		goto tx_drop;
 
@@ -684,13 +732,11 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		goto tx_drop;
 	}
 
-	tx_ind = skb->queue_mapping;
-	ring = priv->tx_ring[tx_ind];
 	if (vlan_tx_tag_present(skb))
 		vlan_tag = vlan_tx_tag_get(skb);
 
 	/* Check available TXBBs And 2K spare for prefetch */
-	if (unlikely(((int)(ring->prod - ring->cons)) >
+	if (unlikely(((int)(ring->prod - ring_cons)) >
 		     ring->size - HEADROOM - MAX_DESC_TXBBS)) {
 		/* every full Tx ring stops queue */
 		netif_tx_stop_queue(ring->tx_queue);
@@ -704,7 +750,8 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		 */
 		wmb();
 
-		if (unlikely(((int)(ring->prod - ring->cons)) <=
+		ring_cons = ACCESS_ONCE(ring->cons);
+		if (unlikely(((int)(ring->prod - ring_cons)) <=
 			     ring->size - HEADROOM - MAX_DESC_TXBBS)) {
 			netif_tx_wake_queue(ring->tx_queue);
 			ring->wake_queue++;
@@ -713,9 +760,11 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 	}
 
+	prefetchw(&ring->tx_queue->dql);
+
 	/* Track current inflight packets for performance analysis */
 	AVG_PERF_COUNTER(priv->pstats.inflight_avg,
-			 (u32) (ring->prod - ring->cons - 1));
+			 (u32)(ring->prod - ring_cons - 1));
 
 	/* Packet is good - grab an index and transmit it */
 	index = ring->prod & ring->size_mask;
@@ -735,31 +784,34 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	tx_info->skb = skb;
 	tx_info->nr_txbb = nr_txbb;
 
+	data = &tx_desc->data;
 	if (lso_header_size)
 		data = ((void *)&tx_desc->lso + ALIGN(lso_header_size + 4,
 						      DS_SIZE));
-	else
-		data = &tx_desc->data;
 
 	/* valid only for none inline segments */
 	tx_info->data_offset = (void *)data - (void *)tx_desc;
 
+	tx_info->inl = inline_ok;
+
 	tx_info->linear = (lso_header_size < skb_headlen(skb) &&
-			   !is_inline(ring->inline_thold, skb, NULL)) ? 1 : 0;
+			   !inline_ok) ? 1 : 0;
 
-	data += skb_shinfo(skb)->nr_frags + tx_info->linear - 1;
+	tx_info->nr_frags = shinfo->nr_frags + tx_info->linear;
+	data += tx_info->nr_frags - 1;
 
-	if (is_inline(ring->inline_thold, skb, &fragptr)) {
-		tx_info->inl = 1;
-	} else {
-		/* Map fragments */
-		for (i = skb_shinfo(skb)->nr_frags - 1; i >= 0; i--) {
+	if (!tx_info->inl) {
+		dma_addr_t dma = 0;
+		u32 byte_count = 0;
+
+		/* Map fragments if any */
+		for (i_frag = shinfo->nr_frags - 1; i_frag >= 0; i_frag--) {
 			struct skb_frag_struct *frag;
-			dma_addr_t dma;
 
-			frag = &skb_shinfo(skb)->frags[i];
+			frag = &shinfo->frags[i_frag];
+			byte_count = skb_frag_size(frag);
 			dma = skb_frag_dma_map(ddev, frag,
-					       0, skb_frag_size(frag),
+					       0, byte_count,
 					       DMA_TO_DEVICE);
 			if (dma_mapping_error(ddev, dma))
 				goto tx_drop_unmap;
@@ -767,14 +819,13 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 			data->addr = cpu_to_be64(dma);
 			data->lkey = cpu_to_be32(mdev->mr.key);
 			wmb();
-			data->byte_count = cpu_to_be32(skb_frag_size(frag));
+			data->byte_count = cpu_to_be32(byte_count);
 			--data;
 		}
 
-		/* Map linear part */
+		/* Map linear part if needed */
 		if (tx_info->linear) {
-			u32 byte_count = skb_headlen(skb) - lso_header_size;
-			dma_addr_t dma;
+			byte_count = skb_headlen(skb) - lso_header_size;
 
 			dma = dma_map_single(ddev, skb->data +
 					     lso_header_size, byte_count,
@@ -787,25 +838,24 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 			wmb();
 			data->byte_count = cpu_to_be32(byte_count);
 		}
-		tx_info->inl = 0;
+		/* tx completion can avoid cache line miss for common cases */
+		tx_info->frag0_dma = dma;
+		tx_info->frag0_byte_count = byte_count;
 	}
 
 	/*
 	 * For timestamping add flag to skb_shinfo and
 	 * set flag for further reference
 	 */
-	if (ring->hwtstamp_tx_type == HWTSTAMP_TX_ON &&
-	    skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
-		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+	tx_info->ts_requested = 0;
+	if (unlikely(ring->hwtstamp_tx_type == HWTSTAMP_TX_ON &&
+		     shinfo->tx_flags & SKBTX_HW_TSTAMP)) {
+		shinfo->tx_flags |= SKBTX_IN_PROGRESS;
 		tx_info->ts_requested = 1;
 	}
 
 	/* Prepare ctrl segement apart opcode+ownership, which depends on
 	 * whether LSO is used */
-	tx_desc->ctrl.vlan_tag = cpu_to_be16(vlan_tag);
-	tx_desc->ctrl.ins_vlan = MLX4_WQE_CTRL_INS_VLAN *
-		!!vlan_tx_tag_present(skb);
-	tx_desc->ctrl.fence_size = (real_size / 16) & 0x3f;
 	tx_desc->ctrl.srcrb_flags = priv->ctrl_flags;
 	if (likely(skb->ip_summed == CHECKSUM_PARTIAL)) {
 		tx_desc->ctrl.srcrb_flags |= cpu_to_be32(MLX4_WQE_CTRL_IP_CSUM |
@@ -826,6 +876,8 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* Handle LSO (TSO) packets */
 	if (lso_header_size) {
+		int i;
+
 		/* Mark opcode as LSO */
 		op_own = cpu_to_be32(MLX4_OPCODE_LSO | (1 << 6)) |
 			((ring->prod & ring->size) ?
@@ -833,15 +885,15 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		/* Fill in the LSO prefix */
 		tx_desc->lso.mss_hdr_size = cpu_to_be32(
-			skb_shinfo(skb)->gso_size << 16 | lso_header_size);
+			shinfo->gso_size << 16 | lso_header_size);
 
 		/* Copy headers;
 		 * note that we already verified that it is linear */
 		memcpy(tx_desc->lso.header, skb->data, lso_header_size);
 
 		priv->port_stats.tso_packets++;
-		i = ((skb->len - lso_header_size) / skb_shinfo(skb)->gso_size) +
-			!!((skb->len - lso_header_size) % skb_shinfo(skb)->gso_size);
+		i = ((skb->len - lso_header_size) / shinfo->gso_size) +
+			!!((skb->len - lso_header_size) % shinfo->gso_size);
 		tx_info->nr_bytes = skb->len + (i - 1) * lso_header_size;
 		ring->packets += i;
 	} else {
@@ -851,16 +903,14 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 			 cpu_to_be32(MLX4_EN_BIT_DESC_OWN) : 0);
 		tx_info->nr_bytes = max_t(unsigned int, skb->len, ETH_ZLEN);
 		ring->packets++;
-
 	}
 	ring->bytes += tx_info->nr_bytes;
 	netdev_tx_sent_queue(ring->tx_queue, tx_info->nr_bytes);
 	AVG_PERF_COUNTER(priv->pstats.tx_pktsz_avg, skb->len);
 
-	if (tx_info->inl) {
-		build_inline_wqe(tx_desc, skb, real_size, &vlan_tag, tx_ind, fragptr);
-		tx_info->inl = 1;
-	}
+	if (tx_info->inl)
+		build_inline_wqe(tx_desc, skb, shinfo, real_size,
+				 &vlan_tag, tx_ind, fragptr);
 
 	if (skb->encapsulation) {
 		struct iphdr *ipv4 = (struct iphdr *)skb_inner_network_header(skb);
@@ -873,35 +923,53 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 	ring->prod += nr_txbb;
 
 	/* If we used a bounce buffer then copy descriptor back into place */
-	if (bounce)
+	if (unlikely(bounce))
 		tx_desc = mlx4_en_bounce_to_desc(priv, ring, index, desc_size);
 
 	skb_tx_timestamp(skb);
 
-	if (ring->bf_enabled && desc_size <= MAX_BF && !bounce && !vlan_tx_tag_present(skb)) {
-		tx_desc->ctrl.bf_qpn |= cpu_to_be32(ring->doorbell_qpn);
+	real_size = (real_size / 16) & 0x3f;
+
+	send_doorbell = !skb->xmit_more || netif_xmit_stopped(ring->tx_queue);
+
+	if (ring->bf_enabled && desc_size <= MAX_BF && !bounce &&
+	    !vlan_tx_tag_present(skb) && send_doorbell) {
+		tx_desc->ctrl.bf_qpn = ring->doorbell_qpn |
+				       cpu_to_be32(real_size);
 
 		op_own |= htonl((bf_index & 0xffff) << 8);
-		/* Ensure new descirptor hits memory
-		* before setting ownership of this descriptor to HW */
+		/* Ensure new descriptor hits memory
+		 * before setting ownership of this descriptor to HW
+		 */
 		wmb();
 		tx_desc->ctrl.owner_opcode = op_own;
 
 		wmb();
 
-		mlx4_bf_copy(ring->bf.reg + ring->bf.offset, (unsigned long *) &tx_desc->ctrl,
-		     desc_size);
+		mlx4_bf_copy(ring->bf.reg + ring->bf.offset,
+			     &tx_desc->ctrl,
+			     desc_size);
 
 		wmb();
 
 		ring->bf.offset ^= ring->bf.buf_size;
 	} else {
-		/* Ensure new descirptor hits memory
-		* before setting ownership of this descriptor to HW */
+		tx_desc->ctrl.vlan_tag = cpu_to_be16(vlan_tag);
+		tx_desc->ctrl.ins_vlan = MLX4_WQE_CTRL_INS_VLAN *
+			!!vlan_tx_tag_present(skb);
+		tx_desc->ctrl.fence_size = real_size;
+
+		/* Ensure new descriptor hits memory
+		 * before setting ownership of this descriptor to HW
+		 */
 		wmb();
 		tx_desc->ctrl.owner_opcode = op_own;
-		wmb();
-		iowrite32be(ring->doorbell_qpn, ring->bf.uar->map + MLX4_SEND_DOORBELL);
+
+		if (send_doorbell) {
+			wmb(); /* ensure owner_opcode is written */
+			iowrite32(ring->doorbell_qpn,
+				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
+		}
 	}
 
 	return NETDEV_TX_OK;
@@ -909,8 +977,8 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 tx_drop_unmap:
 	en_err(priv, "DMA mapping error\n");
 
-	for (i++; i < skb_shinfo(skb)->nr_frags; i++) {
-		data++;
+	while (++i_frag < shinfo->nr_frags) {
+		++data;
 		dma_unmap_page(ddev, (dma_addr_t) be64_to_cpu(data->addr),
 			       be32_to_cpu(data->byte_count),
 			       PCI_DMA_TODEVICE);
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 6a4fc2394cf2..8025e3c0b14e 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -216,13 +216,16 @@ enum cq_type {
 
 struct mlx4_en_tx_info {
 	struct sk_buff *skb;
-	u32 nr_txbb;
-	u32 nr_bytes;
-	u8 linear;
-	u8 data_offset;
-	u8 inl;
-	u8 ts_requested;
-};
+	dma_addr_t	frag0_dma;
+	u32		frag0_byte_count;
+	u32		nr_txbb;
+	u32		nr_bytes;
+	u8		linear;
+	u8		data_offset;
+	u8		inl;
+	u8		ts_requested;
+	u8		nr_frags;
+} ____cacheline_aligned_in_smp;
 
 
 #define MLX4_EN_BIT_DESC_OWN	0x80000000
@@ -253,39 +256,47 @@ struct mlx4_en_rx_alloc {
 };
 
 struct mlx4_en_tx_ring {
+			/* cache line used and dirtied in tx completion
+			 * (mlx4_en_free_tx_buf())
+			 */
+	u32			last_nr_txbb;
+	u32			cons;
+
+			/* cache line used and dirtied in mlx4_en_xmit()
+			 */
+	u32			prod ____cacheline_aligned_in_smp;
+	unsigned long		bytes;
+	unsigned long		packets;
+	unsigned long		tx_csum;
+	struct mlx4_bf		bf;
+	unsigned long		queue_stopped;
+	unsigned long		wake_queue;
+
+			/* Following part should be mostly read
+			 */
+	cpumask_t		affinity_mask;
+	struct mlx4_qp		qp;
 	struct mlx4_hwq_resources wqres;
-	u32 size ; /* number of TXBBs */
-	u32 size_mask;
-	u16 stride;
-	u16 cqn;	/* index of port CQ associated with this ring */
-	u32 prod;
-	u32 cons;
-	u32 buf_size;
-	u32 doorbell_qpn;
-	void *buf;
-	u16 poll_cnt;
-	struct mlx4_en_tx_info *tx_info;
-	u8 *bounce_buf;
-	u8 queue_index;
-	cpumask_t affinity_mask;
-	u32 last_nr_txbb;
-	struct mlx4_qp qp;
-	struct mlx4_qp_context context;
-	int qpn;
-	enum mlx4_qp_state qp_state;
-	struct mlx4_srq dummy;
-	unsigned long bytes;
-	unsigned long packets;
-	unsigned long tx_csum;
-	unsigned long queue_stopped;
-	unsigned long wake_queue;
-	struct mlx4_bf bf;
-	bool bf_enabled;
-	bool bf_alloced;
-	struct netdev_queue *tx_queue;
-	int hwtstamp_tx_type;
-	int inline_thold;
-};
+	u32			size ; /* number of TXBBs */
+	u32			size_mask;
+	u16			stride;
+	u16			cqn;	/* index of port CQ associated with this ring */
+	u32			buf_size;
+	__be32			doorbell_qpn;
+	__be32			mr_key;
+	void			*buf;
+	struct mlx4_en_tx_info	*tx_info;
+	u8			*bounce_buf;
+	struct mlx4_qp_context	context;
+	int			qpn;
+	enum mlx4_qp_state	qp_state;
+	u8			queue_index;
+	bool			bf_enabled;
+	bool			bf_alloced;
+	struct netdev_queue	*tx_queue;
+	int			hwtstamp_tx_type;
+	int			inline_thold;
+} ____cacheline_aligned_in_smp;
 
 struct mlx4_en_rx_desc {
 	/* actual number of entries depends on rx ring stride */
diff --git a/include/linux/mlx4/device.h b/include/linux/mlx4/device.h
index 03b5608a4329..5e5ad07548b8 100644
--- a/include/linux/mlx4/device.h
+++ b/include/linux/mlx4/device.h
@@ -583,7 +583,7 @@ struct mlx4_uar {
 };
 
 struct mlx4_bf {
-	unsigned long		offset;
+	unsigned int		offset;
 	int			buf_size;
 	struct mlx4_uar	       *uar;
 	void __iomem	       *reg;

^ permalink raw reply related

* Re: [PATCH] net: fec: remove memory copy for rx path
From: David Miller @ 2014-09-27 22:56 UTC (permalink / raw)
  To: romieu; +Cc: fugang.duan, Frank.Li, netdev, shawn.guo, bhutchings
In-Reply-To: <20140927113937.GA25265@electric-eye.fr.zoreil.com>

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 27 Sep 2014 13:39:37 +0200

> fugang.duan@freescale.com <fugang.duan@freescale.com> :
>> From: David Miller <davem@davemloft.net> Sent: Saturday, September 27, 2014 5:07 AM
> [...]
>> >We call this rx_copybreak, and there are many drivers you can look at to
>> >see how this works.
>> >
>> >Thanks.
>> 
>> Great suggestion. I will change it for next version.
> 
> Be aware that copybreak module option should be considered legacy.
> 
> There is an ethtool api for it.

Right.

^ permalink raw reply

* Re: [PATCH v1 0/4] net: fec: Code cleanup
From: David Miller @ 2014-09-27 22:55 UTC (permalink / raw)
  To: fugang.duan; +Cc: netdev, Frank.Li
In-Reply-To: <ea65c2c86f264e49940038df21121c9e@BLUPR03MB373.namprd03.prod.outlook.com>

From: "fugang.duan@freescale.com" <fugang.duan@freescale.com>
Date: Sat, 27 Sep 2014 09:51:52 +0000

> From: David Miller <davem@davemloft.net> Sent: Saturday, September 27, 2014 4:06 AM
>>To: Duan Fugang-B38611
>>Cc: netdev@vger.kernel.org; Li Frank-B20596
>>Subject: Re: [PATCH v1 0/4] net: fec: Code cleanup
>>
>>From: Fugang Duan <b38611@freescale.com>
>>Date: Tue, 23 Sep 2014 15:40:54 +0800
>>
>>> This patches does several things:
>>>   - Fixing multiqueue issue.
>>>   - Removing the unnecessary errata workaround.
>>>   - Aligning the data buffer dma map/unmap size.
>>>   - Freeing resource after probe failed.
>>
>>Series applied to net-next, thanks.
>>
>>Please always explicitly state what tree you expect your patches to be
>>applied to in the future.
> 
> No special requirement, just follow the normal rel flow.

That doesn't mean anything to me.

You must state what tree your patches apply properly to, and thus
where you expect them to be intergrated.

^ permalink raw reply

* [PATCH net] ipv6: remove rt6i_genid
From: Hannes Frederic Sowa @ 2014-09-27 22:46 UTC (permalink / raw)
  To: netdev
  Cc: Eric Dumazet, YOSHIFUJI Hideaki, Vlad Yasevich, Nicolas Dichtel,
	Martin Lau

Eric Dumazet noticed that all no-nonexthop or no-gateway routes which
are already marked DST_HOST (e.g. input routes routes) will always be
invalidated during sk_dst_check. Thus per-socket dst caching absolutely
had no effect and early demuxing had no effect.

Thus this patch removes rt6i_genid: fn_sernum already gets modified during
add operations, so we only must ensure we mutate fn_sernum during ipv6
address remove operations. This is a fairly cost extensive operations,
but address removal should not happen that often. Also our mtu update
functions do the same and we heard no complains so far. xfrm policy
changes also cause a call into fib6_flush_trees. Also plug a hole in
rt6_info (no cacheline changes).

I verified via tracing that this change has effect.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: YOSHIFUJI Hideaki <hideaki@yoshifuji.org>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Cc: Martin Lau <kafai@fb.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/net/ip6_fib.h       |  5 +----
 include/net/net_namespace.h | 20 +++-----------------
 net/ipv6/addrconf.c         |  3 ++-
 net/ipv6/addrconf_core.c    |  7 +++++++
 net/ipv6/ip6_fib.c          | 20 ++++++++++++++++++++
 net/ipv6/route.c            |  4 ----
 6 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 9bcb220..cf485f9 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -114,16 +114,13 @@ struct rt6_info {
 	u32				rt6i_flags;
 	struct rt6key			rt6i_src;
 	struct rt6key			rt6i_prefsrc;
-	u32				rt6i_metric;
 
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-	u32				rt6i_genid;
-
+	u32				rt6i_metric;
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
-
 	u8				rt6i_protocol;
 };
 
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 361d260..e0d6466 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -352,26 +352,12 @@ static inline void rt_genid_bump_ipv4(struct net *net)
 	atomic_inc(&net->ipv4.rt_genid);
 }
 
-#if IS_ENABLED(CONFIG_IPV6)
-static inline int rt_genid_ipv6(struct net *net)
-{
-	return atomic_read(&net->ipv6.rt_genid);
-}
-
-static inline void rt_genid_bump_ipv6(struct net *net)
-{
-	atomic_inc(&net->ipv6.rt_genid);
-}
-#else
-static inline int rt_genid_ipv6(struct net *net)
-{
-	return 0;
-}
-
+extern void (*__fib6_flush_trees)(struct net *net);
 static inline void rt_genid_bump_ipv6(struct net *net)
 {
+	if (__fib6_flush_trees)
+		__fib6_flush_trees(net);
 }
-#endif
 
 #if IS_ENABLED(CONFIG_IEEE802154_6LOWPAN)
 static inline struct netns_ieee802154_lowpan *
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3342ee6..3e118df 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4780,10 +4780,11 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 
 		if (ip6_del_rt(ifp->rt))
 			dst_free(&ifp->rt->dst);
+
+		rt_genid_bump_ipv6(net);
 		break;
 	}
 	atomic_inc(&net->ipv6.dev_addr_genid);
-	rt_genid_bump_ipv6(net);
 }
 
 static void ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
diff --git a/net/ipv6/addrconf_core.c b/net/ipv6/addrconf_core.c
index e696045..98cc4cd 100644
--- a/net/ipv6/addrconf_core.c
+++ b/net/ipv6/addrconf_core.c
@@ -8,6 +8,13 @@
 #include <net/addrconf.h>
 #include <net/ip.h>
 
+/* if ipv6 module registers this function is used by xfrm to force all
+ * sockets to relookup their nodes - this is fairly expensive, be
+ * careful
+ */
+void (*__fib6_flush_trees)(struct net *);
+EXPORT_SYMBOL(__fib6_flush_trees);
+
 #define IPV6_ADDR_SCOPE_TYPE(scope)	((scope) << 16)
 
 static inline unsigned int ipv6_addr_scope2type(unsigned int scope)
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 76b7f5e..97b9fa8 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -1605,6 +1605,24 @@ static void fib6_prune_clones(struct net *net, struct fib6_node *fn)
 	fib6_clean_tree(net, fn, fib6_prune_clone, 1, NULL);
 }
 
+static int fib6_update_sernum(struct rt6_info *rt, void *arg)
+{
+	__u32 sernum = *(__u32 *)arg;
+
+	if (rt->rt6i_node &&
+	    rt->rt6i_node->fn_sernum != sernum)
+		rt->rt6i_node->fn_sernum = sernum;
+
+	return 0;
+}
+
+static void fib6_flush_trees(struct net *net)
+{
+	__u32 new_sernum = fib6_new_sernum();
+
+	fib6_clean_all(net, fib6_update_sernum, &new_sernum);
+}
+
 /*
  *	Garbage collection
  */
@@ -1788,6 +1806,8 @@ int __init fib6_init(void)
 			      NULL);
 	if (ret)
 		goto out_unregister_subsys;
+
+	__fib6_flush_trees = fib6_flush_trees;
 out:
 	return ret;
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f23fbd2..bafde82 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -314,7 +314,6 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 	}
 	return rt;
@@ -1098,9 +1097,6 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
-		return NULL;
-
 	if (!rt->rt6i_node || (rt->rt6i_node->fn_sernum != cookie))
 		return NULL;
 
-- 
1.9.3

^ permalink raw reply related

* Re: [RFC PATCH net-next] net: pktgen: packet bursting via skb->xmit_more
From: Eric Dumazet @ 2014-09-27 21:30 UTC (permalink / raw)
  To: Or Gerlitz
  Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
	Eric Dumazet, John Fastabend, Linux Netdev List, Amir Vadai,
	Or Gerlitz
In-Reply-To: <CAJ3xEMhQCf7j48Yn5sjH6jaYYdLQUgOe2hk-OZhhdZ7taOyx+Q@mail.gmail.com>

On Sat, 2014-09-27 at 23:55 +0300, Or Gerlitz wrote:

> mmm, so the numberz sound good, the comment sounds as the basics are
> still not fully behind us (so we have where to improve)... any mlx4
> patch you want to share?

Sure, I am definitely not playing around on mlx4 a Saturday to throw
away my work ;)

I'll send a patch maybe today if I am not too bored.

^ permalink raw reply

* [PATCH] wireless: Remove unnessary write over of register 11 in rt2800lib.c
From: Nicholas Krause @ 2014-09-27 21:11 UTC (permalink / raw)
  To: IvDoorn-Re5JQEeQqe8AvxtiuMwx3w
  Cc: helmut.schaa-gM/Ye1E23mwN+BqQ9rBEUg,
	linville-2XuSBdqkA4R54TAoqtyWWQ,
	linux-wireless-u79uwXL29TY76Z2rM5mHXA,
	users-poMEt7QlJxcwIE2E9O76wjtx2kNaKg5H,
	netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

This removes the unnecessary  overwrite of register 11 in the function,
rt2800_config_channel as we are already writing a correct value to the
register with rt2800_rfcsr_write(rt2x00dev,11.rfcsr).

Signed-off-by: Nicholas Krause <yocto6-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/wireless/rt2x00/rt2800lib.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2800lib.c b/drivers/net/wireless/rt2x00/rt2800lib.c
index 893c9d5..fb0ae38 100644
--- a/drivers/net/wireless/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/rt2x00/rt2800lib.c
@@ -2787,8 +2787,6 @@ static void rt2800_config_channel_rf55xx(struct rt2x00_dev *rt2x00dev,
 
 	if (rf->channel <= 14) {
 		rt2800_rfcsr_write(rt2x00dev, 10, 0x90);
-		/* FIXME: RF11 owerwrite ? */
-		rt2800_rfcsr_write(rt2x00dev, 11, 0x4A);
 		rt2800_rfcsr_write(rt2x00dev, 12, 0x52);
 		rt2800_rfcsr_write(rt2x00dev, 13, 0x42);
 		rt2800_rfcsr_write(rt2x00dev, 22, 0x40);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [RFC PATCH net-next] net: pktgen: packet bursting via skb->xmit_more
From: Or Gerlitz @ 2014-09-27 20:59 UTC (permalink / raw)
  To: Alexei Starovoitov, Eric Dumazet
  Cc: David S. Miller, Jesper Dangaard Brouer, John Fastabend,
	Linux Netdev List
In-Reply-To: <1411692382-8898-1-git-send-email-ast@plumgrid.com>

On Fri, Sep 26, 2014 at 3:46 AM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> This patch demonstrates the effect of delaying update of HW tailptr.
> (based on earlier patch by Jesper)
>
> burst=1 is a default. It sends one packet with xmit_more=false
> burst=2 sends one packet with xmit_more=true and
>         2nd copy of the same packet with xmit_more=false
> burst=3 sends two copies of the same packet with xmit_more=true and
>         3rd copy with xmit_more=false


Guys, just to be sure, this pktgen config uses only one TX ring, right?


> Performance with ixgbe:
>
> usec 30:
> burst=1  tx:9.2 Mpps
> burst=2  tx:13.6 Mpps
> burst=3  tx:14.5 Mpps full 10G line rate
>
> usec 1 (default):
> burst=1,4,100 tx:3.9 Mpps
>
> usec 0:
> burst=1  tx:4.9 Mpps
> burst=2  tx:6.6 Mpps
> burst=3  tx:7.9 Mpps
> burst=4  tx:8.7 Mpps
> burst=8  tx:10.3 Mpps
> burst=128  tx:12.4 Mpps
>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
> ---
>
> tx queue size, irq affinity left in default.
> pause frames are off.
>
> Nice to finally see line rate generated by one cpu
>
> Comparing to Jesper patch this one amortizes the cost
> of spin_lock and atomic_inc by doing HARD_TX_LOCK and
> atomic_add(N) once across N packets.

^ permalink raw reply

* Re: [RFC PATCH net-next] net: pktgen: packet bursting via skb->xmit_more
From: Or Gerlitz @ 2014-09-27 20:55 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Alexei Starovoitov, David S. Miller, Jesper Dangaard Brouer,
	Eric Dumazet, John Fastabend, Linux Netdev List, Amir Vadai,
	Or Gerlitz
In-Reply-To: <1411850590.15768.6.camel@edumazet-glaptop2.roam.corp.google.com>

On Sat, Sep 27, 2014 at 11:43 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-09-26 at 00:42 -0700, Eric Dumazet wrote:
>
>> By the way with this patch, I now reach 10 Mpps on mlx4
>>
>> base line : 5 Mpps
>>
>> + skb->xmit_more and quick hack in pjtgen (spinlock/unlock per packet)
>> -> 7 Mpps
>>
>> + burst of 16 packets, no spinlock per packet -> 10 Mpps
>
> With careful study of mlx4 driver to remove false sharing, I now get 14
> Mpps.
>
> (Note they have a special feature to 'inline' small packets in tx
> descriptors : If this is used, max rate is lower, because cpu spend more
> cycles to perform the copies)
>
> I find worrying driver authors do not know how to properly use a ring
> buffer, and do not place the producer and consumer indexes in separate
> cache lines.

mmm, so the numberz sound good, the comment sounds as the basics are
still not fully behind us (so we have where to improve)... any mlx4
patch you want to share?

^ permalink raw reply

* Re: [RFC PATCH net-next] net: pktgen: packet bursting via skb->xmit_more
From: Eric Dumazet @ 2014-09-27 20:43 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Jesper Dangaard Brouer, Eric Dumazet,
	John Fastabend, netdev, Amir Vadai, Or Gerlitz
In-Reply-To: <1411717322.16953.99.camel@edumazet-glaptop2.roam.corp.google.com>

On Fri, 2014-09-26 at 00:42 -0700, Eric Dumazet wrote:

> By the way with this patch, I now reach 10 Mpps on mlx4 
> 
> base line : 5 Mpps
> 
> + skb->xmit_more and quick hack in pjtgen (spinlock/unlock per packet)
> -> 7 Mpps
> 
> + burst of 16 packets, no spinlock per packet -> 10 Mpps

With careful study of mlx4 driver to remove false sharing, I now get 14
Mpps.

(Note they have a special feature to 'inline' small packets in tx
descriptors : If this is used, max rate is lower, because cpu spend more
cycles to perform the copies)

I find worrying driver authors do not know how to properly use a ring
buffer, and do not place the producer and consumer indexes in separate
cache lines.

^ permalink raw reply

* Re: [PATCH v3 5/7] net: Add GSO support for UDP tunnels with checksum
From: Or Gerlitz @ 2014-09-27 20:39 UTC (permalink / raw)
  To: Tom Herbert; +Cc: David Miller, netdev@vger.kernel.org
In-Reply-To: <CA+mtBx-FqE70TROzxi=-B9DKaUsMs-=KZk3XbY7_qhOk3b2aNQ@mail.gmail.com>

On Sat, Sep 27, 2014 at 12:57 AM, Tom Herbert <therbert@google.com> wrote:
> On Fri, Sep 26, 2014 at 2:05 PM, Or Gerlitz <or.gerlitz@gmail.com> wrote:
>> On Thu, Jun 5, 2014 at 3:20 AM, Tom Herbert <therbert@google.com> wrote:

>>> Added a new netif feature for GSO_UDP_TUNNEL_CSUM. This indicates
>>> that a device is capable of computing the UDP checksum in the
>>> encapsulating header of a UDP tunnel.

>> Do we have upstream driver that supports GSO_UDP_TUNNEL_CSUM? did you
>> had such driver/patch while doing this patches? when a driver
>> advertizes that bit, should they look over the xmit path on the new
>> encap_hdr_csum bit?

> No, no, and encap_hdr_csum should only be set with
> SKB_GSO_UDP_TUNNEL_CSUM or SKB_GSO_GRE_CSUM.

I'm still trying to dig the bigger picture w.r.t checksum of the outer
UDP packet from the patches -- if I got it right, once these patches
were picked upstream, there's a scheme where the kernel either
computes this checksum or let the device do that - when they advertize
NETIF_F_GSO_YYY_CSUM  and in that case
(skb->encap_hdr_csum == true) should be interpreted as a directive to
do that, right?

So what happens when the device isn't capable to compute that checksum
(e.g they don't set _GSO_UDP_TUNNEL_CSUM) but they do advertize the
GSO_UDP_TUNNEL bit?

I was worried that we can run into this scheme - the stack computes
the outer checksum for the giant 64K UDP chunck that encapsulate a 64K
TCP segment, but when the NIC will issue the segmentation, they will
likely to just repeat ~40 times (64K/1500) the original udp checksum
for the packets they send, which will be treated as bad checksum on
the receiving end, bad.

> find . -name '*.[ch]' -exec fgrep -l GSO_UDP_TUNNEL_CSUM {} \;
> returns nothing.

grepping I know, I was just hoping you were able to test such a sensitive change
with HW that cause both code parts (outer udp checksum offloaded vs.
non-offloaded) to be exercised

> find . -name '*.[ch]' -exec fgrep -l GSO_UDP_TUNNEL {} \;
> returns
>
> ./ethernet/intel/i40e/i40e_main.c
> ./ethernet/broadcom/bnx2x/bnx2x_main.c
> ./ethernet/qlogic/qlcnic/qlcnic_main.c
> ./ethernet/mellanox/mlx4/en_netdev.c
> ./ethernet/emulex/benet/be_main.c

cool, I see here four 40Gbs NICs that support GSO offloading of VXLAN
traffic, each of them can serve you for testing new developments you
do in that area.

Or.

^ permalink raw reply

* Re: [PATCH net-next] r8169:add support for RTL8168EP
From: Francois Romieu @ 2014-09-27 20:39 UTC (permalink / raw)
  To: Chun-Hao Lin; +Cc: netdev, nic_swsd, linux-kernel
In-Reply-To: <1411726731-15304-1-git-send-email-hau@realtek.com>

Chun-Hao Lin <hau@realtek.com> :
> RTL8168EP is Realtek PCIe Gigabit Ethernet controller.
> It is a successor chip of RTL8168DP.
> 
> This patch add support for this chip.

It does more than that.

Did Hayes review it ?

> Signed-off-by: Chun-Hao Lin <hau@realtek.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 611 +++++++++++++++++++++++++++++------
>  1 file changed, 514 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 1d81238..0ead9a7 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -155,6 +155,9 @@ enum mac_version {
>  	RTL_GIGA_MAC_VER_46,
>  	RTL_GIGA_MAC_VER_47,
>  	RTL_GIGA_MAC_VER_48,
> +	RTL_GIGA_MAC_VER_49,
> +	RTL_GIGA_MAC_VER_50,
> +	RTL_GIGA_MAC_VER_51,
>  	RTL_GIGA_MAC_NONE   = 0xff,
>  };
>  
> @@ -302,6 +305,15 @@ static const struct {
>  	[RTL_GIGA_MAC_VER_48] =
>  		_R("RTL8107e",		RTL_TD_1, FIRMWARE_8107E_2,
>  							JUMBO_1K, false),
> +	[RTL_GIGA_MAC_VER_49] =
> +		_R("RTL8168ep/8111ep",	RTL_TD_1, NULL,
> +							JUMBO_9K, false),
> +	[RTL_GIGA_MAC_VER_50] =
> +		_R("RTL8168ep/8111ep",	RTL_TD_1, NULL,
> +							JUMBO_9K, false),
> +	[RTL_GIGA_MAC_VER_51] =
> +		_R("RTL8168ep/8111ep",	RTL_TD_1, NULL,
> +							JUMBO_9K, false),
>  };
>  #undef _R
>  
> @@ -400,6 +412,10 @@ enum rtl_registers {
>  	FuncEvent	= 0xf0,
>  	FuncEventMask	= 0xf4,
>  	FuncPresetState	= 0xf8,
> +	IBCR0           = 0xf8,
> +	IBCR2           = 0xf9,
> +	IBIMR0          = 0xfa,
> +	IBISR0          = 0xfb,
>  	FuncForceEvent	= 0xfc,
>  };
>  
> @@ -467,6 +483,7 @@ enum rtl8168_registers {
>  #define ERIAR_EXGMAC			(0x00 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_MSIX			(0x01 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_ASF			(0x02 << ERIAR_TYPE_SHIFT)
> +#define ERIAR_OOB			(0x02 << ERIAR_TYPE_SHIFT)
>  #define ERIAR_MASK_SHIFT		12
>  #define ERIAR_MASK_0001			(0x1 << ERIAR_MASK_SHIFT)
>  #define ERIAR_MASK_0011			(0x3 << ERIAR_MASK_SHIFT)
> @@ -935,93 +952,10 @@ static const struct rtl_cond name = {			\
>  							\
>  static bool name ## _check(struct rtl8169_private *tp)
>  
> -DECLARE_RTL_COND(rtl_ocpar_cond)
> -{
> -	void __iomem *ioaddr = tp->mmio_addr;
> -
> -	return RTL_R32(OCPAR) & OCPAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate
patch. You are adding extra noise and thus making this stuff harder
to review than it should be.

> -
> -static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> -{
> -	void __iomem *ioaddr = tp->mmio_addr;
> -
> -	RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> -	return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20) ?
> -		RTL_R32(OCPDR) : ~0;
> -}

(this one is modified)

> -
> -static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
> -{
> -	void __iomem *ioaddr = tp->mmio_addr;
> -
> -	RTL_W32(OCPDR, data);
> -	RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> -
> -	rtl_udelay_loop_wait_low(tp, &rtl_ocpar_cond, 100, 20);
> -}

(this one is modified)

> -
> -DECLARE_RTL_COND(rtl_eriar_cond)
> -{
> -	void __iomem *ioaddr = tp->mmio_addr;
> -
> -	return RTL_R32(ERIAR) & ERIAR_FLAG;
> -}

Feel free to move this function around but please do it in a separate
patch.

> -
> -static void rtl8168_oob_notify(struct rtl8169_private *tp, u8 cmd)
> -{
> -	void __iomem *ioaddr = tp->mmio_addr;
> -
> -	RTL_W8(ERIDR, cmd);
> -	RTL_W32(ERIAR, 0x800010e8);
> -	msleep(2);
> -
> -	if (!rtl_udelay_loop_wait_low(tp, &rtl_eriar_cond, 100, 5))
> -		return;
> -
> -	ocp_write(tp, 0x1, 0x30, 0x00000001);
> -}

This one is modified but it is also modified for the existing 8168DP.

Mantra: please do it in a separate patch.

> -
>  #define OOB_CMD_RESET		0x00
>  #define OOB_CMD_DRIVER_START	0x05
>  #define OOB_CMD_DRIVER_STOP	0x06
>  
> -static u16 rtl8168_get_ocp_reg(struct rtl8169_private *tp)
> -{
> -	return (tp->mac_version == RTL_GIGA_MAC_VER_31) ? 0xb8 : 0x10;
> -}
> -
> -DECLARE_RTL_COND(rtl_ocp_read_cond)
> -{
> -	u16 reg;
> -
> -	reg = rtl8168_get_ocp_reg(tp);
> -
> -	return ocp_read(tp, 0x0f, reg) & 0x00000800;
> -}

(this one is simply moved around)

> -
> -static void rtl8168_driver_start(struct rtl8169_private *tp)
> -{
> -	rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> -
> -	rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);
> -}

(this one is modified)

> -
> -static void rtl8168_driver_stop(struct rtl8169_private *tp)
> -{
> -	rtl8168_oob_notify(tp, OOB_CMD_DRIVER_STOP);
> -
> -	rtl_msleep_loop_wait_low(tp, &rtl_ocp_read_cond, 10, 10);
> -}

(this one is modified)

> -
> -static int r8168dp_check_dash(struct rtl8169_private *tp)
> -{
> -	u16 reg = rtl8168_get_ocp_reg(tp);
> -
> -	return (ocp_read(tp, 0x0f, reg) & 0x00008000) ? 1 : 0;
> -}

(this one is modified)

[...]
> @@ -1329,6 +1277,45 @@ static void rtl_w1w0_eri(struct rtl8169_private *tp, int addr, u32 mask, u32 p,
>  	rtl_eri_write(tp, addr, mask, (val & ~m) | p, type);
>  }
>  
> +static u32 ocp_read(struct rtl8169_private *tp, u8 mask, u16 reg)
> +{
> +	void __iomem *ioaddr = tp->mmio_addr;
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_51) {
> +		return rtl_eri_read(tp, reg, ERIAR_OOB);
> +	} else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_31) {
> +		RTL_W32(OCPAR, ((u32)mask & 0x0f) << 12 | (reg & 0x0fff));
> +		return rtl_udelay_loop_wait_high(tp, &rtl_ocpar_cond, 100, 20)
> +		? RTL_R32(OCPDR) : ~0;

Please:
- use a local "bool rc" variable for rtl_udelay_loop_wait_high status
  so that you can line things correctly.
- use a switch case
- croak in the default case

> +	}
> +
> +	return ~0;
> +}
> +
> +static void ocp_write(struct rtl8169_private *tp, u8 mask, u16 reg, u32 data)
> +{
> +	void __iomem *ioaddr = tp->mmio_addr;
> +
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_51) {
> +		rtl_eri_write(tp, reg, ((u32)mask & 0x0f) << ERIAR_MASK_SHIFT,
> +		data, ERIAR_OOB);

It should line up after the opening parenthesis in "rtl_eri_write("

> +	} else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_31) {
> +		RTL_W32(OCPDR, data);
> +		RTL_W32(OCPAR, OCPAR_FLAG | ((u32)mask & 0x0f) << 12 |
> +		(reg & 0x0fff));

It should line up after the opening parenthesis in "RTL_W32("

Please:
- use a switch case
- croak in the default case

You may state in a comment that this function is only designed for 8168DP
and derivatives (whence the reduced switch case). 

[...]
> @@ -1361,6 +1348,103 @@ static u8 rtl8168d_efuse_read(struct rtl8169_private *tp, int reg_addr)
[...]
> +static void rtl8168_driver_start(struct rtl8169_private *tp)
> +{
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_51) {
> +		u32 tmp;
> +
> +		ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START);
> +		tmp = ocp_read(tp, 0x01, 0x30);
> +		tmp |= 0x01;
> +		ocp_write(tp, 0x01, 0x30, tmp);
> +		rtl_msleep_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10, 10);
> +	} else if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> +		   tp->mac_version == RTL_GIGA_MAC_VER_31) {
> +		rtl8168_oob_notify(tp, OOB_CMD_DRIVER_START);
> +		rtl_msleep_loop_wait_high(tp, &rtl_ocp_read_cond, 10, 10);

switch case + croak on default please.

[snip]
> @@ -3724,6 +3819,143 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp)
>  	rtl_writephy(tp, 0x1f, 0x0000);
>  }
>  
> +static void rtl8168ep_1_hw_phy_config(struct rtl8169_private *tp)
> +{
> +	rtl_apply_firmware(tp);

?

You did not specify any firmware information in rtl_chip_infos.

It some firmware is supposed to help, please be sure to add the relevant
#define FIRMWARE_8xyz as well.

[...]
> +static void rtl8168ep_2_hw_phy_config(struct rtl8169_private *tp)
> +{
> +	rtl_apply_firmware(tp);

See above.

[...]
> @@ -4265,7 +4511,7 @@ static void r810x_pll_power_up(struct rtl8169_private *tp)
>  		break;
>  	case RTL_GIGA_MAC_VER_47:
>  	case RTL_GIGA_MAC_VER_48:
> -		RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> +		RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
>  		break;

It's welcome but it should be done in a different patch.

[...]
> @@ -4339,8 +4585,11 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>  
>  	if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
>  	     tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> -	     tp->mac_version == RTL_GIGA_MAC_VER_31) &&
> -	    r8168dp_check_dash(tp)) {
> +	     tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> +	     r8168_check_dash(tp)) {
>  		return;

Unrelated behavior change for RTL_GIGA_MAC_VER_2[78]. Different patch.

>  	}
>  
> @@ -4369,12 +4618,16 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>  	case RTL_GIGA_MAC_VER_33:
>  	case RTL_GIGA_MAC_VER_45:
>  	case RTL_GIGA_MAC_VER_46:
> +	case RTL_GIGA_MAC_VER_50:
> +	case RTL_GIGA_MAC_VER_51:
>  		RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);
>  		break;
>  	case RTL_GIGA_MAC_VER_40:
>  	case RTL_GIGA_MAC_VER_41:
> +	case RTL_GIGA_MAC_VER_49:
>  		rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0x00000000,
>  			     0xfc000000, ERIAR_EXGMAC);
> +		RTL_W8(PMCH, RTL_R8(PMCH) & ~0x80);

Unrelated behavior change for RTL_GIGA_MAC_VER_4[01] (8168g, huh ?).
-> Different patch.

[...]
> @@ -4395,10 +4648,14 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
>  		break;
>  	case RTL_GIGA_MAC_VER_45:
>  	case RTL_GIGA_MAC_VER_46:
> -		RTL_W8(PMCH, RTL_R8(PMCH) | 0xC0);
> +	case RTL_GIGA_MAC_VER_50:
> +	case RTL_GIGA_MAC_VER_51:
> +		RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);

See above.

>  		break;
>  	case RTL_GIGA_MAC_VER_40:
>  	case RTL_GIGA_MAC_VER_41:
> +	case RTL_GIGA_MAC_VER_49:
> +		RTL_W8(PMCH, RTL_R8(PMCH) | 0xc0);
>  		rtl_w1w0_eri(tp, 0x1a8, ERIAR_MASK_1111, 0xfc000000,
>  			     0x00000000, ERIAR_EXGMAC);

See above.

[...]
> @@ -7366,9 +7766,13 @@ static void rtl_remove_one(struct pci_dev *pdev)
>  	struct net_device *dev = pci_get_drvdata(pdev);
>  	struct rtl8169_private *tp = netdev_priv(dev);
>  
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_31) {
> +	if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> +	     r8168_check_dash(tp)) {

It does not line up correctly (one space excess).

Unrelated behavior change for RTL_GIGA_MAC_VER_{27, 28, 31}. Different patch.

>  		rtl8168_driver_stop(tp);
>  	}
>  
[...]
> @@ -7703,11 +8113,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (tp->mac_version == RTL_GIGA_MAC_VER_45 ||
>  	    tp->mac_version == RTL_GIGA_MAC_VER_46 ||
>  	    tp->mac_version == RTL_GIGA_MAC_VER_47 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_48) {
> +	    tp->mac_version == RTL_GIGA_MAC_VER_48 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	    tp->mac_version == RTL_GIGA_MAC_VER_51) {
>  		u16 mac_addr[3];
>  
> -		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xE0, ERIAR_EXGMAC);
> -		*(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xE4, ERIAR_EXGMAC);
> +		*(u32 *)&mac_addr[0] = rtl_eri_read(tp, 0xe0, ERIAR_EXGMAC);
> +		*(u16 *)&mac_addr[2] = rtl_eri_read(tp, 0xe4, ERIAR_EXGMAC);

It's welcome but it brings noise in the current patch. Different patch.

>  
>  		if (is_valid_ether_addr((u8 *)mac_addr))
>  			rtl_rar_set(tp, (u8 *)mac_addr);
> @@ -7780,9 +8193,13 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			   rtl_chip_infos[chipset].jumbo_tx_csum ? "ok" : "ko");
>  	}
>  
> -	if (tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> -	    tp->mac_version == RTL_GIGA_MAC_VER_31) {
> +	if ((tp->mac_version == RTL_GIGA_MAC_VER_27 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_28 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_31 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_49 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_50 ||
> +	     tp->mac_version == RTL_GIGA_MAC_VER_51) &&
> +	     r8168_check_dash(tp)) {

See above. Unrelated change, etc.

>  		rtl8168_driver_start(tp);
>  	}

-- 
Ueimor

^ permalink raw reply

* CONGRATULATION!!!
From: TOTAL OIL AND GAS COMPANY LONDON @ 2014-09-27 21:26 UTC (permalink / raw)


[-- Attachment #1: Type: text/plain, Size: 19 bytes --]



-- 
 ....APPROVAL

[-- Attachment #2: (n).docx --]
[-- Type: application/vnd.openxmlformats-officedocument.wordprocessingml.document, Size: 194695 bytes --]

^ permalink raw reply

* Re: [PATCH 9/11 V2 NEXT] rtlwifi: rtl8188ee: Update driver to match Realtek release of 06282014
From: Francois Romieu @ 2014-09-27 19:49 UTC (permalink / raw)
  To: Larry Finger; +Cc: linville, linux-wireless, troy_tan, netdev
In-Reply-To: <1411836726-12699-2-git-send-email-Larry.Finger@lwfinger.net>


Larry Finger <Larry.Finger@lwfinger.net> :
[...]
> diff --git a/drivers/net/wireless/rtlwifi/core.c b/drivers/net/wireless/rtlwifi/core.c
> index 40738e3..d30f416 100644
> --- a/drivers/net/wireless/rtlwifi/core.c
> +++ b/drivers/net/wireless/rtlwifi/core.c
> @@ -28,6 +28,7 @@
>  #include "cam.h"
>  #include "base.h"
>  #include "ps.h"
> +#include "pwrseqcmd.h"
>  
>  #include "btcoexist/rtl_btc.h"
>  #include <linux/firmware.h>
> @@ -1700,3 +1701,100 @@ const struct ieee80211_ops rtl_ops = {
>  };
>  EXPORT_SYMBOL_GPL(rtl_ops);
>  
> +/*	Description:
> + *		This routine deals with the Power Configuration CMD
> + *		 parsing for RTL8723/RTL8188E Series IC.
> + *	Assumption:
> + *		We should follow specific format that was released from HW SD.

"released" is a bit misleading if it does not mean "publicly released".

> + */
> +bool rtl_hal_pwrseqcmdparsing(struct rtl_priv *rtlpriv, u8 cut_version,
> +			      u8 faversion, u8 interface_type,
> +			      struct wlan_pwr_cfg pwrcfgcmd[])
> +{
> +	struct wlan_pwr_cfg cfg_cmd = {0};
> +	bool polling_bit = false;
> +	u32 ary_idx = 0;

	u32 i;

> +	u8 value = 0;
> +	u32 offset = 0;

Useless init.

> +	u32 polling_count = 0;

Wrong scope.

> +	u32 max_polling_cnt = 5000;

Inverted xmas tree for declaration.

> +
> +	do {

	while (1) {

> +		cfg_cmd = pwrcfgcmd[ary_idx];
> +		RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> +			 "rtl_hal_pwrseqcmdparsing(): offset(%#x),cut_msk(%#x), famsk(%#x), interface_msk(%#x), base(%#x), cmd(%#x), msk(%#x), value(%#x)\n",
                                                                ^^^ missing space

> +			 GET_PWR_CFG_OFFSET(cfg_cmd),
> +					    GET_PWR_CFG_CUT_MASK(cfg_cmd),
> +			 GET_PWR_CFG_FAB_MASK(cfg_cmd),
> +					      GET_PWR_CFG_INTF_MASK(cfg_cmd),
> +			 GET_PWR_CFG_BASE(cfg_cmd), GET_PWR_CFG_CMD(cfg_cmd),
> +			 GET_PWR_CFG_MASK(cfg_cmd), GET_PWR_CFG_VALUE(cfg_cmd));
> +
> +		if ((GET_PWR_CFG_FAB_MASK(cfg_cmd)&faversion) &&
> +		    (GET_PWR_CFG_CUT_MASK(cfg_cmd)&cut_version) &&
                                                 ^^^ missing spaces

> +		    (GET_PWR_CFG_INTF_MASK(cfg_cmd)&interface_type)) {
> +			switch (GET_PWR_CFG_CMD(cfg_cmd)) {
> +			case PWR_CMD_READ:
> +				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> +					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_READ\n");

It should line up after the parenthesis.

> +				break;
> +			case PWR_CMD_WRITE:
> +				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> +					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_WRITE\n");
> +				offset = GET_PWR_CFG_OFFSET(cfg_cmd);
> +
> +				/*Read the value from system register*/
> +				value = rtl_read_byte(rtlpriv, offset);
> +				value &= (~(GET_PWR_CFG_MASK(cfg_cmd)));

Excess parenthsis.

> +				value |= (GET_PWR_CFG_VALUE(cfg_cmd) &
> +					  GET_PWR_CFG_MASK(cfg_cmd));

Excess parenthsis.

> +
> +				/*Write the value back to sytem register*/
> +				rtl_write_byte(rtlpriv, offset, value);
> +				break;
> +			case PWR_CMD_POLLING:
> +				RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
> +					"rtl_hal_pwrseqcmdparsing(): PWR_CMD_POLLING\n");

It should line up after the parenthesis.

> +				polling_bit = false;
> +				offset = GET_PWR_CFG_OFFSET(cfg_cmd);
> +
> +				do {
> +					value = rtl_read_byte(rtlpriv, offset);
> +
> +					value &= GET_PWR_CFG_MASK(cfg_cmd);
> +					if (value ==
> +					    (GET_PWR_CFG_VALUE(cfg_cmd) &
> +					     GET_PWR_CFG_MASK(cfg_cmd)))
> +						polling_bit = true;
> +					else
> +						udelay(10);
> +
> +					if (polling_count++ > max_polling_cnt)
> +						return false;
> +				} while (!polling_bit);

Simple "for" loop in disguise.

An helper function may help.

Imnsho the whole do { more, more, more } while (1) concept is broken.

[...]
> diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
> index c764fff..d9ea9d0 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
> +++ b/drivers/net/wireless/rtlwifi/rtl8188ee/def.h
[...]
> @@ -225,44 +218,37 @@ enum power_polocy_config {
>  };
>  
>  enum interface_select_pci {
> -	INTF_SEL1_MINICARD,
> -	INTF_SEL0_PCIE,
> -	INTF_SEL2_RSV,
> -	INTF_SEL3_RSV,
> +	INTF_SEL1_MINICARD = 0,
> +	INTF_SEL0_PCIE = 1,
> +	INTF_SEL2_RSV = 2,
> +	INTF_SEL3_RSV = 3,

Please use tabs to line things up.

[...]
> +	HAL_FW_C2H_CMD_READ_MACREG = 0,
> +	HAL_FW_C2H_CMD_READ_BBREG = 1,
> +	HAL_FW_C2H_CMD_READ_RFREG = 2,
> +	HAL_FW_C2H_CMD_READ_EEPROM = 3,
> +	HAL_FW_C2H_CMD_READ_EFUSE = 4,

See above.

[...]
> diff --git a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
> index f8daa61..2aa34d9 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8188ee/dm.c
[...]
> -			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
> -				      value32);
> +			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
> +				      MASKDWORD, value32);

It lacks of consistency, see the *NICE* marker below.

>  			value32 = (ele_c & 0x000003C0) >> 6;
> -			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, value32);
> +			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS,
> +				      value32);

?

>  			value32 = ((iqk_result_x * ele_d) >> 7) & 0x01;
> -			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), value32);
> +			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24),
> +				      value32);
>  			break;
>  		case RF90_PATH_B:
>  			value32 = (ele_d << 22)|((ele_c & 0x3F)<<16) | ele_a;
> -			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL,
> -				      MASKDWORD, value32);
> +			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE, MASKDWORD,
> +				      value32);

*NICE*

[...]
> @@ -210,16 +209,20 @@ static void rtl88e_set_iqk_matrix(struct ieee80211_hw *hw,
>  	} else {
>  		switch (rfpath) {
>  		case RF90_PATH_A:
> -			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBAL, MASKDWORD,
> -				      ofdmswing_table[ofdm_index]);
> -			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);
> -			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(24), 0x00);
> +			rtl_set_bbreg(hw, ROFDM0_XATXIQIMBALANCE,
> +				      MASKDWORD, ofdmswing_table[ofdm_index]);
> +			rtl_set_bbreg(hw, ROFDM0_XCTXAFE,
> +				      MASKH4BITS, 0x00);

			rtl_set_bbreg(hw, ROFDM0_XCTXAFE, MASKH4BITS, 0x00);

> +			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
> +				      BIT(24), 0x00);

			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(24), 0x00);

>  			break;
>  		case RF90_PATH_B:
> -			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBAL, MASKDWORD,
> -				      ofdmswing_table[ofdm_index]);
> -			rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);
> -			rtl_set_bbreg(hw, ROFDM0_ECCATHRES, BIT(28), 0x00);
> +			rtl_set_bbreg(hw, ROFDM0_XBTXIQIMBALANCE,
> +				      MASKDWORD, ofdmswing_table[ofdm_index]);
> +			rtl_set_bbreg(hw, ROFDM0_XDTXAFE,
> +				      MASKH4BITS, 0x00);

			rtl_set_bbreg(hw, ROFDM0_XDTXAFE, MASKH4BITS, 0x00);

> +			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD,
> +				      BIT(28), 0x00);

			rtl_set_bbreg(hw, ROFDM0_ECCATHRESHOLD, BIT(28), 0x00);

[...]
> @@ -263,46 +266,75 @@ void rtl88e_dm_txpower_track_adjust(struct ieee80211_hw *hw,
>  			 (pwr_val << 24);
>  }
>  
> -
> -static void rtl88e_chk_tx_track(struct ieee80211_hw *hw,
> -				enum pwr_track_control_method method,
> -				u8 rfpath, u8 index)
> +static void dm_tx_pwr_track_set_pwr(struct ieee80211_hw *hw,
> +				    enum pwr_track_control_method method,
> +				    u8 rfpath, u8 channel_mapped_index)
>  {
>  	struct rtl_priv *rtlpriv = rtl_priv(hw);
> -	struct rtl_phy *rtlphy = &(rtlpriv->phy);
> +	struct rtl_phy *rtlphy = &rtlpriv->phy;
>  	struct rtl_dm *rtldm = rtl_dm(rtl_priv(hw));
> -	int jj = rtldm->swing_idx_cck;
> -	int i;
>  
>  	if (method == TXAGC) {
> -		if (rtldm->swing_flag_ofdm == true ||
> -		    rtldm->swing_flag_cck == true) {
> -			u8 chan = rtlphy->current_channel;
> -			rtl88e_phy_set_txpower_level(hw, chan);
> +		if (rtldm->swing_flag_ofdm ||
> +		    rtldm->swing_flag_cck) {

Single line.

> +			rtl88e_phy_set_txpower_level(hw,
> +						     rtlphy->current_channel);
>  			rtldm->swing_flag_ofdm = false;
>  			rtldm->swing_flag_cck = false;
>  		}
>  	} else if (method == BBSWING) {
>  		if (!rtldm->cck_inch14) {
> -			for (i = 0; i < 8; i++)
> -				rtl_write_byte(rtlpriv, 0xa22 + i,
> -					       cck_tbl_ch1_13[jj][i]);
> +			rtl_write_byte(rtlpriv, 0xa22,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][0]);
> +			rtl_write_byte(rtlpriv, 0xa23,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][1]);
> +			rtl_write_byte(rtlpriv, 0xa24,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][2]);
> +			rtl_write_byte(rtlpriv, 0xa25,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][3]);
> +			rtl_write_byte(rtlpriv, 0xa26,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][4]);
> +			rtl_write_byte(rtlpriv, 0xa27,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][5]);
> +			rtl_write_byte(rtlpriv, 0xa28,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][6]);
> +			rtl_write_byte(rtlpriv, 0xa29,
> +				       cck_tbl_ch1_13[rtldm->swing_idx_cck][7]);

?

I hope someone got the "-R" flag of patch wrong.

>  		} else {
> -			for (i = 0; i < 8; i++)
> -				rtl_write_byte(rtlpriv, 0xa22 + i,
> -					       cck_tbl_ch14[jj][i]);
> +			rtl_write_byte(rtlpriv, 0xa22,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][0]);
> +			rtl_write_byte(rtlpriv, 0xa23,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][1]);
> +			rtl_write_byte(rtlpriv, 0xa24,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][2]);
> +			rtl_write_byte(rtlpriv, 0xa25,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][3]);
> +			rtl_write_byte(rtlpriv, 0xa26,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][4]);
> +			rtl_write_byte(rtlpriv, 0xa27,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][5]);
> +			rtl_write_byte(rtlpriv, 0xa28,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][6]);
> +			rtl_write_byte(rtlpriv, 0xa29,
> +				       cck_tbl_ch14[rtldm->swing_idx_cck][7]);

Sic.

>  		}
>  
>  		if (rfpath == RF90_PATH_A) {
> -			long x = rtlphy->iqk_matrix[index].value[0][0];
> -			long y = rtlphy->iqk_matrix[index].value[0][1];
> -			u8 indx = rtldm->swing_idx_ofdm[rfpath];
> -			rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
> +			rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
> +					      rfpath, rtlphy->iqk_matrix
> +					      [channel_mapped_index].
> +					      value[0][0],
> +					      rtlphy->iqk_matrix
> +					      [channel_mapped_index].
> +					      value[0][1]);
>  		} else if (rfpath == RF90_PATH_B) {
> -			u8 indx = rtldm->swing_idx_ofdm[rfpath];
> -			long x = rtlphy->iqk_matrix[indx].value[0][4];
> -			long y = rtlphy->iqk_matrix[indx].value[0][5];
> -			rtl88e_set_iqk_matrix(hw, indx, rfpath, x, y);
> +			rtl88e_set_iqk_matrix(hw, rtldm->swing_idx_ofdm[rfpath],
> +					      rfpath, rtlphy->iqk_matrix
> +					      [channel_mapped_index].
> +					      value[0][4],
> +					      rtlphy->iqk_matrix
> +					      [channel_mapped_index].
> +					      value[0][5]);

:o(

I stop here. I don't enjoy playing the bad cop and this patch is feeding crap
into the kernel.

-- 
Ueimor

^ permalink raw reply

* Re: [PATCH net-next 0/5] udp: Generalize GSO for UDP tunnels
From: Or Gerlitz @ 2014-09-27 19:26 UTC (permalink / raw)
  To: Tom Herbert; +Cc: David Miller, Linux Netdev List
In-Reply-To: <CA+mtBx-0ov5W_xVsgEJsw7o=A9CYSq6ZC_C34=MMsUxgzJWp_g@mail.gmail.com>

On Sat, Sep 27, 2014 at 2:04 AM, Tom Herbert <therbert@google.com> wrote:
> On Fri, Sep 26, 2014 at 1:16 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Fri, Sep 26, 2014 at 7:22 PM, Tom Herbert <therbert@google.com> wrote:
>> [...]
>>> Notes:
>>>   - GSO for GRE/UDP where GRE checksum is enabled does not work.
>>>     Handling this will require some special case code.
>>>   - Software GSO now supports many varieties of encapsulation with
>>>     SKB_GSO_UDP_TUNNEL{_CSUM}. We still need a mechanism to query
>>>     for device support of particular combinations (I intend to
>>>     add ndo_gso_check for that).
>>
>> Tom,
>>
>> As I wrote you earlier on another thread/s, fact is that there are
>> upstream drivers who advertize SKB_GSO_UDP_TUNNEL and aren't capable @
>> this point to issue proper HW segmentation of something which isn't
>> VXLAN.
>>
>> Just to make sure, this series isn't expected to introduce a
>> regression, right? we don't expect the stack to attempt and xmit a
>> large 64KB UDP packet which isn't vxlan through these devices.

> I am planning to post ndo_gso_check shortly. These patches should not
> cause a regression with currently deployed functionality (VXLAN).

Can you sum up (please) in 1-2 liner what is the trick to avoid such
regression? that is what/where is the knob that would prevent such
giant chunk to be sent down to a NIC driver which does advertize
SKB_GSO_UDP_TUNNEL?


>>>   - MPLS seems to be the only previous user of inner_protocol. I don't
>>>     believe these patches can affect that. For supporting GSO with
>>>     MPLS over UDP, the inner_protocol should be set using the
>>>     helper functions in this patch.
>>>   - GSO for L2TP/UDP should also be straightforward now.
>>
>>> Tested GRE, IPIP, and SIT over fou as well as VLXAN. This was
>>> done using 200 TCP_STREAMs in netperf.
>> [...]
>>>    VXLAN
>>>       TCP_STREAM TSO enabled on tun interface
>>>         16.42% TX CPU utilization
>>>         23.66% RX CPU utilization
>>>         9081 Mbps
>>>       TCP_STREAM TSO disabled on tun interface
>>>         30.32% TX CPU utilization
>>>         30.55% RX CPU utilization
>>>         9185 Mbps
>>
>> so TSO disabled has better BW vs TSO enabled?
>>
> Yes, I've noticed that on occasion, it does seem like TSO disabled
> tends to get a little more throughput. I see this with plain GRE, so I
> don't think it's directly related to fou or these patches. I suppose
> there may be some subtle interactions with BQL or something like that.
> I'd probably want to repro this on some other devices at some point to
> dig deeper.
>
>>>    Baseline (no encp, TSO and LRO enabled)
>>>       TCP_STREAM
>>>         11.85% TX CPU utilization
>>>         15.13% RX CPU utilization
>>>         9452 Mbps
>>
>> I would strongly recommend to have a far better baseline when
>> developing and testing these changes in the stack in the form of 40Gbs
>> NICs.
>>
> The only point of putting the baseline was to show that encapsulation
> with GSO/GRO/checksum-unnec-conversion is in the ballpark of
> performance with native traffic which was a goal.

under (over...) 10Gbs, in the ballpark indeed.

We know nothing what would happen with baseline of 38Gbs (SB 40Gbs
NIC) 56Gbs (two bonded ports of 40Gbs NIC on PCIe gen3) or 100Gbs
(tomorrow's NIC HW, probably coming up next year)

> So I'm pretty happy
> with this performance right now, although it probably does mean remote
> checksum offload won't show so impressive results with this test (TX
> csum with data in case isn't so expensive).
> Out of curiosity, why do you think using 40Gbs is far better for a baseline?

Oh, simply b/c with 40Gbs NICs, the baseline I expect for few sessions
(1,2,4 or 200 as you did) of plain TCP is four times better vs. your
current one (38Gbs vs 9.5Gbs) and this should pose a harder challenge
for the GSO/encapsulating stack to catch up with, agree?

Or.

^ permalink raw reply

* [PATCH] IB/ipoib: order:1 failure in ipoib_cm_alloc_rx_skb causes softlockup
From: Venkat Venkatsubra @ 2014-09-27 18:20 UTC (permalink / raw)
  To: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA
  Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
	venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA

In ipoib_cm_alloc_rx_skb
  skb = dev_alloc_skb(IPOIB_CM_HEAD_SIZE + 12);
results in order:1 allocation because IPOIB_CM_HEAD_SIZE is defined as:
	IPOIB_ENCAP_LEN     = 4,
	IPOIB_CM_MTU  = 0x10000 - 0x10, /* padding to align header to 16 */
	IPOIB_CM_BUF_SIZE	= IPOIB_CM_MTU  + IPOIB_ENCAP_LEN,
	IPOIB_CM_HEAD_SIZE  = IPOIB_CM_BUF_SIZE % PAGE_SIZE,

For a 4Kbytes PAGE_SIZE IPOIB_CM_HEAD_SIZE ends up as
(65536 - 16 + 4) % 4096 = 4084 bytes resulting in
dev_alloc_skb(4084 + 12) or dev_alloc_skb(4096) or order:1 allocation.

This fix avoids this by redefining IPOIB_CM_HEAD_SIZE to whatever maximum
that can fit in a order:0 allocation and adjusting number of scatter/gather
elements to accommodate the rest of the bytes.

IPOIB_CM_RX_SG is incremented by 1 to accommodate one extra SG element.

Since using NET_SKB_PAD in IPOIB_CM_HEAD_SIZE now gives the below
compilation error due to max()
"braced-group within expression allowed only inside a function"
actual number of SG elements used is now computed in a variable.

Here is a stack trace of an order:1 failure:
 kswapd0: page allocation failure. order:1, mode:0x20
 Pid: 273, comm: kswapd0 Not tainted 2.6.32-400.11.1.el5uek #1
 Call Trace:
  <IRQ>  [<ffffffff810ddf74>] __alloc_pages_nodemask+0x524/0x595
  [<ffffffff8110da3f>] kmem_getpages+0x4f/0xf4
  [<ffffffff8110dc3c>] fallback_alloc+0x158/0x1ce
  [<ffffffff8110ddd3>] ____cache_alloc_node+0x121/0x134
  [<ffffffff8110e3f3>] kmem_cache_alloc_node_notrace+0x84/0xb9
  [<ffffffff8110e46e>] __kmalloc_node+0x46/0x73
  [<ffffffff813b9aa8>] ? __alloc_skb+0x72/0x13d
  [<ffffffff813b9aa8>] __alloc_skb+0x72/0x13d
  [<ffffffff813b9bdb>] alloc_skb+0x13/0x15
  [<ffffffff813b9f11>] dev_alloc_skb+0x1b/0x38
  [<ffffffffa029e722>] ipoib_cm_alloc_rx_skb+0x31/0x1de [ib_ipoib]
  [<ffffffffa029fd04>] ipoib_cm_handle_rx_wc+0x3a1/0x5b8 [ib_ipoib]
  [<ffffffffa0191bdc>] ? mlx4_ib_free_srq_wqe+0x27/0x54 [mlx4_ib]
  [<ffffffffa01894d4>] ? mlx4_ib_poll_cq+0x620/0x65e [mlx4_ib]
  [<ffffffff813b9015>] ? __kfree_skb+0x79/0x7e
  [<ffffffffa029e9f7>] ? netif_tx_lock+0x44/0x71 [ib_ipoib]
  [<ffffffffa029ae97>] ipoib_poll+0x87/0x128 [ib_ipoib]
  [<ffffffff813c4b69>] net_rx_action+0xc6/0x1cd
  [<ffffffff8105e8cd>] __do_softirq+0xd7/0x19e
  [<ffffffff810aefdc>] ? handle_IRQ_event+0x66/0x120
  [<ffffffff81012eec>] call_softirq+0x1c/0x30
  [<ffffffff81014695>] do_softirq+0x46/0x89
  [<ffffffff8105e752>] irq_exit+0x3b/0x7a
  [<ffffffff8145bea1>] do_IRQ+0x99/0xb0
  [<ffffffff81012713>] ret_from_intr+0x0/0x11
  <EOI>  [<ffffffff812379a3>] ? radix_tree_delete+0x8f/0x194
  [<ffffffffa03693b3>] ? __nfs_access_zap_cache+0x75/0xb0 [nfs]
  [<ffffffff81207d05>] ? ima_inode_free+0x35/0x55
  [<ffffffff8112fd9c>] ? __destroy_inode+0x26/0x66
  [<ffffffff8112fdf2>] ? destroy_inode+0x16/0x44
  [<ffffffff81130074>] ? dispose_list+0xb2/0xe1
  [<ffffffff81130251>] ? shrink_icache_memory+0x1ae/0x1e0
  [<ffffffff810e3f83>] ? shrink_slab+0xe1/0x153
  [<ffffffff810e5063>] ? kswapd+0x3dd/0x516
  [<ffffffff810e26f3>] ? isolate_pages_global+0x0/0x1ba
  [<ffffffff810432be>] ? need_resched+0x23/0x2d
  [<ffffffff81077030>] ? autoremove_wake_function+0x0/0x3d
  [<ffffffff810e4c86>] ? kswapd+0x0/0x516
  [<ffffffff81076c87>] ? kthread+0x6e/0x76
  [<ffffffff81012dea>] ? child_rip+0xa/0x20
  [<ffffffff81076c19>] ? kthread+0x0/0x76
  [<ffffffff81012de0>] ? child_rip+0x0/0x20

Signed-off-by: Venkat Venkatsubra <venkat.x.venkatsubra-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib.h      | 11 +++++++++--
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 21 +++++++++++++--------
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  7 ++++++-
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index 3edce61..e0b5a63 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -61,6 +61,11 @@ enum ipoib_flush_level {
 	IPOIB_FLUSH_HEAVY
 };
 
+/* 12 bytes added to align the IP header to a multiple of 16 bytes
+ * after IPoIB adds 4 byte header.
+ */
+#define	IPOIB_CM_HEAD_SIZE SKB_MAX_HEAD(NET_SKB_PAD + 12)
+
 enum {
 	IPOIB_ENCAP_LEN		  = 4,
 
@@ -69,8 +74,8 @@ enum {
 
 	IPOIB_CM_MTU		  = 0x10000 - 0x10, /* padding to align header to 16 */
 	IPOIB_CM_BUF_SIZE	  = IPOIB_CM_MTU  + IPOIB_ENCAP_LEN,
-	IPOIB_CM_HEAD_SIZE	  = IPOIB_CM_BUF_SIZE % PAGE_SIZE,
-	IPOIB_CM_RX_SG		  = ALIGN(IPOIB_CM_BUF_SIZE, PAGE_SIZE) / PAGE_SIZE,
+	/* +1 to accommodate residual data in the last SG element */
+	IPOIB_CM_RX_SG	    = ALIGN(IPOIB_CM_BUF_SIZE, PAGE_SIZE)/PAGE_SIZE + 1,
 	IPOIB_RX_RING_SIZE	  = 256,
 	IPOIB_TX_RING_SIZE	  = 128,
 	IPOIB_MAX_QUEUE_SIZE	  = 8192,
@@ -543,6 +548,8 @@ int ipoib_set_dev_features(struct ipoib_dev_priv *priv, struct ib_device *hca);
 /* We don't support UC connections at the moment */
 #define IPOIB_CM_SUPPORTED(ha)   (ha[0] & (IPOIB_FLAGS_RC))
 
+extern int ipoib_cm_rx_sg;
+
 #ifdef CONFIG_INFINIBAND_IPOIB_CM
 
 extern int ipoib_max_conn_qp;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index 933efce..0b5154f6 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -122,13 +122,13 @@ static int ipoib_cm_post_receive_nonsrq(struct net_device *dev,
 
 	wr->wr_id = id | IPOIB_OP_CM | IPOIB_OP_RECV;
 
-	for (i = 0; i < IPOIB_CM_RX_SG; ++i)
+	for (i = 0; i < ipoib_cm_rx_sg; ++i)
 		sge[i].addr = rx->rx_ring[id].mapping[i];
 
 	ret = ib_post_recv(rx->qp, wr, &bad_wr);
 	if (unlikely(ret)) {
 		ipoib_warn(priv, "post recv failed for buf %d (%d)\n", id, ret);
-		ipoib_cm_dma_unmap_rx(priv, IPOIB_CM_RX_SG - 1,
+		ipoib_cm_dma_unmap_rx(priv, ipoib_cm_rx_sg - 1,
 				      rx->rx_ring[id].mapping);
 		dev_kfree_skb_any(rx->rx_ring[id].skb);
 		rx->rx_ring[id].skb = NULL;
@@ -199,7 +199,7 @@ static void ipoib_cm_free_rx_ring(struct net_device *dev,
 
 	for (i = 0; i < ipoib_recvq_size; ++i)
 		if (rx_ring[i].skb) {
-			ipoib_cm_dma_unmap_rx(priv, IPOIB_CM_RX_SG - 1,
+			ipoib_cm_dma_unmap_rx(priv, ipoib_cm_rx_sg - 1,
 					      rx_ring[i].mapping);
 			dev_kfree_skb_any(rx_ring[i].skb);
 		}
@@ -263,7 +263,7 @@ static struct ib_qp *ipoib_cm_create_rx_qp(struct net_device *dev,
 
 	if (!ipoib_cm_has_srq(dev)) {
 		attr.cap.max_recv_wr  = ipoib_recvq_size;
-		attr.cap.max_recv_sge = IPOIB_CM_RX_SG;
+		attr.cap.max_recv_sge = ipoib_cm_rx_sg;
 	}
 
 	return ib_create_qp(priv->pd, &attr);
@@ -382,7 +382,8 @@ static int ipoib_cm_nonsrq_init_rx(struct net_device *dev, struct ib_cm_id *cm_i
 	spin_unlock_irq(&priv->lock);
 
 	for (i = 0; i < ipoib_recvq_size; ++i) {
-		if (!ipoib_cm_alloc_rx_skb(dev, rx->rx_ring, i, IPOIB_CM_RX_SG - 1,
+		if (!ipoib_cm_alloc_rx_skb(dev, rx->rx_ring, i,
+					   ipoib_cm_rx_sg - 1,
 					   rx->rx_ring[i].mapping,
 					   GFP_KERNEL)) {
 			ipoib_warn(priv, "failed to allocate receive buffer %d\n", i);
@@ -1553,16 +1554,20 @@ int ipoib_cm_dev_init(struct net_device *dev)
 
 	ipoib_dbg(priv, "max_srq_sge=%d\n", attr.max_srq_sge);
 
-	attr.max_srq_sge = min_t(int, IPOIB_CM_RX_SG, attr.max_srq_sge);
+	attr.max_srq_sge = min_t(int, ipoib_cm_rx_sg, attr.max_srq_sge);
 	ipoib_cm_create_srq(dev, attr.max_srq_sge);
 	if (ipoib_cm_has_srq(dev)) {
-		priv->cm.max_cm_mtu = attr.max_srq_sge * PAGE_SIZE - 0x10;
+		int no_skb_frags = attr.max_srq_sge - 1;
+		u32 maxmtu;
+
+		maxmtu = no_skb_frags * PAGE_SIZE + IPOIB_CM_HEAD_SIZE - 0x10;
+		priv->cm.max_cm_mtu = min_t(int, maxmtu, IPOIB_CM_MTU);
 		priv->cm.num_frags  = attr.max_srq_sge;
 		ipoib_dbg(priv, "max_cm_mtu = 0x%x, num_frags=%d\n",
 			  priv->cm.max_cm_mtu, priv->cm.num_frags);
 	} else {
 		priv->cm.max_cm_mtu = IPOIB_CM_MTU;
-		priv->cm.num_frags  = IPOIB_CM_RX_SG;
+		priv->cm.num_frags  = ipoib_cm_rx_sg;
 	}
 
 	ipoib_cm_init_rx_wr(dev, &priv->cm.rx_wr, priv->cm.rx_sge);
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1310acf..50fb941 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -73,6 +73,8 @@ module_param_named(debug_level, ipoib_debug_level, int, 0644);
 MODULE_PARM_DESC(debug_level, "Enable debug tracing if > 0");
 #endif
 
+int	ipoib_cm_rx_sg;
+
 struct ipoib_path_iter {
 	struct net_device *dev;
 	struct ipoib_path  path;
@@ -1257,7 +1259,6 @@ static void ipoib_neigh_hash_uninit(struct net_device *dev)
 	wait_for_completion(&priv->ntbl.deleted);
 }
 
-
 int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
 {
 	struct ipoib_dev_priv *priv = netdev_priv(dev);
@@ -1719,6 +1720,10 @@ static void ipoib_remove_one(struct ib_device *device)
 static int __init ipoib_init_module(void)
 {
 	int ret;
+	int ipoib_cm_sg_len;
+
+	ipoib_cm_sg_len = IPOIB_CM_BUF_SIZE - IPOIB_CM_HEAD_SIZE;
+	ipoib_cm_rx_sg = ALIGN(ipoib_cm_sg_len, PAGE_SIZE)/PAGE_SIZE + 1;
 
 	ipoib_recvq_size = roundup_pow_of_two(ipoib_recvq_size);
 	ipoib_recvq_size = min(ipoib_recvq_size, IPOIB_MAX_QUEUE_SIZE);
-- 
1.8.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox