Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next 1/2] sunvnet: Process Rx data packets in a BH handler
From: Sowmini Varadhan @ 2014-10-01 19:39 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, raghuram.kothakota, netdev
In-Reply-To: <1412190559.16704.59.camel@edumazet-glaptop2.roam.corp.google.com>

On (10/01/14 12:09), Eric Dumazet wrote:
> > -
> > +	/* BH context cannot call netif_receive_skb */
> > +	netif_rx_ni(skb);
> 
> Really ? What about the standard and less expensive netif_receive_skb ?

I can't use netif_receive_skb in this case:
the TCP retransmit timers are softirq context. They can pre-empt here, 
and result in a deadlock on socket locks. E.g.,

tcp_write_timer+0xc/0xa0 <-- wants sk_lock
call_timer_fn+0x24/0x120
run_timer_softirq+0x214/0x2a0
__do_softirq+0xb8/0x200
do_softirq+0x8c/0xc0
local_bh_enable+0xac/0xc0
ip_finish_output+0x254/0x4a0
ip_output+0xc4/0xe0
ip_local_out+0x2c/0x40
ip_queue_xmit+0x140/0x3c0
tcp_transmit_skb+0x448/0x740
tcp_write_xmit+0x220/0x480
__tcp_push_pending_frames+0x38/0x100
tcp_rcv_established+0x214/0x780
tcp_v4_do_rcv+0x154/0x300
tcp_v4_rcv+0x6cc/0xa60   <-- takes sk_lock
  :
netif_receive_skb
 

Ideally I would have liked to  use netif_receive_skb (it boosts perf)
but I had to back off for this reason.

> > +
> > +	struct mutex            vnet_rx_mutex; /* serializes rx_workq */
> > +	struct work_struct      rx_work;
> > +	struct workqueue_struct *rx_workq;
> > +
> >  };
> 
> Could you describe in the changelog why all this is needed ?

So I gave a short summary in the cover letter, but more details

- processing packets in ldc_rx context risks live-lock
- I experimented with a few things, including NAPI, and just using a simple tasklet
  to take care of the data packet handling. With Both NAPI and tasklet, I'm able
  to use netif_receive_skb safely, however, mpstat shows that one CPU ends up
  doing all the processing, and scaling was inhibited.
- further, with  NAPI the budget gets in the way. 

Regarding your other comments"
  "You basically found a way to overcome NAPI standard limits (budget of 64)"
As I said in the cover letter, coercing a budget on sunvnet ends up actually
hurting perf significantly, as we end up sending additional stop/start messages.
To achieve that budget, we'd have to keep a lot more state in vnet to remember
the position in the stream but *not* send a STOP/START, and instead resume
at the next napi_schedule from where we left off.

Doing all this would end up just re-inventing much of the code in process_backlog
anyway.

--Sowmini

 

^ permalink raw reply

* Re: [PATCH 1/1 net-next] tcp: add __init to tcp_init_mem
From: David Miller @ 2014-10-01 19:41 UTC (permalink / raw)
  To: fabf; +Cc: linux-kernel, kuznet, jmorris, yoshfuji, kaber, netdev
In-Reply-To: <1412180870-7599-1-git-send-email-fabf@skynet.be>

From: Fabian Frederick <fabf@skynet.be>
Date: Wed,  1 Oct 2014 18:27:50 +0200

> tcp_init_mem is only called by __init tcp_init.
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied, thank you.

^ permalink raw reply

* Re: [PATCH net-next 2/2] sunvnet: vnet_start_xmit() must hold a refcnt on port.
From: Sowmini Varadhan @ 2014-10-01 19:44 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, raghuram.kothakota, netdev
In-Reply-To: <1412190324.16704.57.camel@edumazet-glaptop2.roam.corp.google.com>

On (10/01/14 12:05), Eric Dumazet wrote:
> 
> Hmpff... This calls for rcu protection here !
> 

I did consider that, but given that the lists containing the ports are
accessed in multiple contexts, some of which can sleep, and given that
the vnet port is similar in spirit to the net_device, I followed the
net_device model of dev_put etc.

--Sowmini

^ permalink raw reply

* Re: [PATCH 1/1 net-next] inet: frags: add __init to ip4_frags_ctl_register
From: David Miller @ 2014-10-01 19:46 UTC (permalink / raw)
  To: fabf; +Cc: linux-kernel, kuznet, jmorris, yoshfuji, kaber, netdev
In-Reply-To: <1412183937-16055-1-git-send-email-fabf@skynet.be>

From: Fabian Frederick <fabf@skynet.be>
Date: Wed,  1 Oct 2014 19:18:57 +0200

> ip4_frags_ctl_register is only called by __init ipfrag_init
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied.

^ permalink raw reply

* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jesper Dangaard Brouer @ 2014-10-01 19:47 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: Tom Herbert, David Miller, Linux Netdev List, Eric Dumazet,
	Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
	Alexander Duyck, John Fastabend, Dave Taht,
	Toke Høiland-Jørgensen, brouer
In-Reply-To: <542C4E0D.4050404@mojatatu.com>

On Wed, 01 Oct 2014 14:55:09 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> On 10/01/14 13:28, Jesper Dangaard Brouer wrote:
> 
> > Thus, code is activated only when q->qlen is >= 1. And I have already
> > shown that we see a win with just bulking 2 packets:
> 
> If you can get 2 packets, indeed you win. If you can on average get >1
> over a long period, you still win.
> You have clearly demonstrated you can do that with traffic
> generators (udp or in kernel pktgen). I was more worried about the
> common use case scenario (handwaved as 1-24 TCP streams).
> 
> The key here is: *if you never hit bulking* then the cost is
> _per packet_ for sch_direct_xmit bypass.
> Question is what is that cost for the common case as defined above?
> Can you hit a bulk level >1 on 1-24 TCP streams?
> I would be happy if your answer is *yes*. 

Answer is yes.  It is very easy with simple netperf TCP_STREAM to cause
queueing >1 packet in the qdisc layer.  If tuned (according to my blog,
unloading netfilter etc.) then a single netperf TCP_STREAM will max out
10Gbit/s and cause a standing queue.

I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never
saw a standing queue of 1 packet in my testing.  Either the backlog
area is high 100-200 packets, or 0 backlog.  (With fake pktgen/trafgen
style tests, it's possible to cause 1000 backlog).


> If your answer is no (since
> it is hard to achieve) - then how far off is it from before your
> patches (since now you have added at minimal a branch check).
> I think it is fair for you to quantify that, no?
> Feature is still useful for the other cases.
> 
> Note:
> This is what i referred to as the "no animals were hurt during the
> making of these patches" statement. I am sorry again for raining on
> the parade.

I'm starting to think this patch is the most carefully tested patch on
netdev for a very very long time... I'm getting tired of this testing
going on for so long.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply

* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: David Miller @ 2014-10-01 19:47 UTC (permalink / raw)
  To: brouer
  Cc: jhs, therbert, netdev, eric.dumazet, hannes, fw, dborkman,
	alexander.duyck, john.r.fastabend, dave.taht, toke
In-Reply-To: <20141001192840.5679a671@redhat.com>

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 1 Oct 2014 19:28:40 +0200

> Thus, code is activated only when q->qlen is >= 1. And I have already
> shown that we see a win with just bulking 2 packets:
>  "Subject: qdisc/UDP_STREAM: measuring effect of qdisc bulk dequeue"
>  http://thread.gmane.org/gmane.linux.network/331152/focus=331154
> 
> I actually believe, that I have already measured and shown that going
> though the bulk dequeue is a win, this should show direct_xmit vs.
> bulking:
>  "Subject: qdisc/trafgen: Measuring effect of qdisc bulk dequeue, with trafgen"
>  http://thread.gmane.org/gmane.linux.network/331152

+1 Amen...

^ permalink raw reply

* Re: [PATCH 1/1 net-next] cipso: add __init to cipso_v4_cache_init
From: David Miller @ 2014-10-01 19:47 UTC (permalink / raw)
  To: fabf; +Cc: linux-kernel, kuznet, jmorris, yoshfuji, kaber, netdev
In-Reply-To: <1412184603-17294-1-git-send-email-fabf@skynet.be>

From: Fabian Frederick <fabf@skynet.be>
Date: Wed,  1 Oct 2014 19:30:03 +0200

> cipso_v4_cache_init is only called by __init cipso_v4_init
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Applied.

^ permalink raw reply

* Re: [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context.
From: David Miller @ 2014-10-01 19:50 UTC (permalink / raw)
  To: sowmini.varadhan; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20141001185604.GG17706@oracle.com>

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Wed, 1 Oct 2014 14:56:04 -0400

> 
> The existing sunvnet implementation does all the packet interception
> in LDC interrupt context. Patch 1 of this series moves the data
> processing to a bottom-half handler.
> 
> Some context for the reasons for choosing a BH handler over NAPI:
>    A NAPI (or simple tasklet) based implementation provides softirq
>    context, which allows the driver to safely invoke netif_receive_skb()
>    to deliver the packet to the IP stack. But in the case of sunvnet,
>    we are already receiving multiple packets for a single ldc_rx
>    interrupt, so the budget-based softirq-vs-polling infra does not
>    provide a significant optimization. Rather, it can get in the way,
>    if we constrain the vnet-rx path to a poorly chosen budget, and force
>    ourselves to send a STOPPED/START ldc exchange needlessly.
> 
>    A BH Rx handler is a simpler way to  avoid the weaknesses of processing
>    packets in LDC interrupt context, and also provides Rx load-spreading
>    across multiple CPUs.

I think you want to re-evaluate this considering napi_gro_receive() which
is what you should really be calling in a NAPI driver.

^ permalink raw reply

* Re: [PATCH net-next 2/2] sunvnet: vnet_start_xmit() must hold a refcnt on port.
From: David Miller @ 2014-10-01 19:52 UTC (permalink / raw)
  To: david.stevens; +Cc: sowmini.varadhan, raghuram.kothakota, netdev
In-Reply-To: <542C56A5.4070805@oracle.com>

From: David L Stevens <david.stevens@oracle.com>
Date: Wed, 01 Oct 2014 15:31:49 -0400

> 
> 
> On 10/01/2014 03:23 PM, Sowmini Varadhan wrote:
>> On (10/01/14 15:06), David L Stevens wrote:
>>>
>>> This "vp->switch_port" addition doesn't appear to be related to the port refcnt
>>> change, and doesn't allow for multiple switch ports.
>> 
>> The switch_port is the connection to Dom0. Do you envision us having more than
>> one switch_port? How?
> 
> While Dom0 might only create one port with the "switch" flag, the flag just means
> "I can reach anybody" and is not inherently unique. I don't think an attached
> VM should assume there is always only one; it prevents multipath load balancing
> kinds of things in the future.
> 
> Also, there is the broader point that this sort of change should be a separate patch.
> It isn't required for fixing the dangling reference -- it is an independent change.

Multiple switch ports are absolutely allowed by the protocol spec and can
provide the suggested facilities David mentioned, don't prevent them from
being used.

^ permalink raw reply

* Re: [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context.
From: Sowmini Varadhan @ 2014-10-01 19:55 UTC (permalink / raw)
  To: David Miller; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20141001.155009.2277009117294992988.davem@davemloft.net>

On 10/01/2014 03:50 PM, David Miller wrote:

>>
>>     A BH Rx handler is a simpler way to  avoid the weaknesses of processing
>>     packets in LDC interrupt context, and also provides Rx load-spreading
>>     across multiple CPUs.
>
> I think you want to re-evaluate this considering napi_gro_receive() which
> is what you should really be calling in a NAPI driver.


Sorry I dont follow the suggestion.

If I make this a NAPI driver that uses napi_gro_receive, I would
still have to deal with a budget, right?

--Sowmini

^ permalink raw reply

* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jamal Hadi Salim @ 2014-10-01 20:05 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Tom Herbert, David Miller, Linux Netdev List, Eric Dumazet,
	Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
	Alexander Duyck, John Fastabend, Dave Taht,
	Toke Høiland-Jørgensen
In-Reply-To: <20141001214700.18b16387@redhat.com>

On 10/01/14 15:47, Jesper Dangaard Brouer wrote:

>
> Answer is yes.  It is very easy with simple netperf TCP_STREAM to cause
> queueing >1 packet in the qdisc layer.

If that is the case, I withdraw any doubts i had.
Can you please specify this in your commit logs for patch 0?

> If tuned (according to my blog,
> unloading netfilter etc.) then a single netperf TCP_STREAM will max out
> 10Gbit/s and cause a standing queue.
>

You should describe such tuning in the patch log (hard to read
blogs for more than 30 seconds; write a paper if you want to provide
more details).

> I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never
> saw a standing queue of 1 packet in my testing.  Either the backlog
> area is high 100-200 packets, or 0 backlog.  (With fake pktgen/trafgen
> style tests, it's possible to cause 1000 backlog).

It would be nice to actually collect such stats. Monitoring the backlog
via dumping qdisc stats is a good start - but actually keeping traces
of average bulk size is more useful.

cheers,
jamal

^ permalink raw reply

* Re: [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context.
From: David Miller @ 2014-10-01 20:15 UTC (permalink / raw)
  To: sowmini.varadhan; +Cc: raghuram.kothakota, netdev
In-Reply-To: <542C5C37.1040409@oracle.com>

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Wed, 01 Oct 2014 15:55:35 -0400

> On 10/01/2014 03:50 PM, David Miller wrote:
> 
>>>
>>>     A BH Rx handler is a simpler way to avoid the weaknesses of processing
>>>     packets in LDC interrupt context, and also provides Rx load-spreading
>>>     across multiple CPUs.
>>
>> I think you want to re-evaluate this considering napi_gro_receive()
>> which
>> is what you should really be calling in a NAPI driver.
> 
> 
> Sorry I dont follow the suggestion.
> 
> If I make this a NAPI driver that uses napi_gro_receive, I would
> still have to deal with a budget, right?

Absolutely, and YOU MUST, because the budget keeps one device from
hogging the RX packet input path from other devices on a given cpu.

^ permalink raw reply

* Re: [PATCH net-next 2/2] sunvnet: vnet_start_xmit() must hold a refcnt on port.
From: Eric Dumazet @ 2014-10-01 20:15 UTC (permalink / raw)
  To: Sowmini Varadhan; +Cc: davem, raghuram.kothakota, netdev
In-Reply-To: <20141001194403.GM17706@oracle.com>

On Wed, 2014-10-01 at 15:44 -0400, Sowmini Varadhan wrote:
> On (10/01/14 12:05), Eric Dumazet wrote:
> > 
> > Hmpff... This calls for rcu protection here !
> > 
> 
> I did consider that, but given that the lists containing the ports are
> accessed in multiple contexts, some of which can sleep, and given that
> the vnet port is similar in spirit to the net_device, I followed the
> net_device model of dev_put etc.

dev_put() uses per cpu variables, an order of magnitude faster than a
atomic put/get :(

^ permalink raw reply

* Re: [PATCH net-next 2/2] sunvnet: vnet_start_xmit() must hold a refcnt on port.
From: Sowmini Varadhan @ 2014-10-01 20:19 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: davem, raghuram.kothakota, netdev
In-Reply-To: <1412194526.16704.61.camel@edumazet-glaptop2.roam.corp.google.com>

On 10/01/2014 04:15 PM, Eric Dumazet wrote:
> On Wed, 2014-10-01 at 15:44 -0400, Sowmini Varadhan wrote:
>> On (10/01/14 12:05), Eric Dumazet wrote:
>>>
>>> Hmpff... This calls for rcu protection here !
>>>
>>
>> I did consider that, but given that the lists containing the ports are
>> accessed in multiple contexts, some of which can sleep, and given that
>> the vnet port is similar in spirit to the net_device, I followed the
>> net_device model of dev_put etc.
>
> dev_put() uses per cpu variables, an order of magnitude faster than a
> atomic put/get :(

I could make this a per-cpu variable, it would not be too hard to change
vnet_put/hold etc. That's not a problem.

--Sowmini

^ permalink raw reply

* Re: [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context.
From: Sowmini Varadhan @ 2014-10-01 20:23 UTC (permalink / raw)
  To: David Miller; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20141001.161508.1823792090990427608.davem@davemloft.net>

On (10/01/14 16:15), David Miller wrote:
> > 
> > If I make this a NAPI driver that uses napi_gro_receive, I would
> > still have to deal with a budget, right?
> 
> Absolutely, and YOU MUST, because the budget keeps one device from
> hogging the RX packet input path from other devices on a given cpu.

yes, but limiting the budget of sk_buffs read mid-way during descriptor
read is deadly to perf because of the ensuing LDC stop/start exchange -
ends up being even worse than the baseline.

Doesnt the netif_rx/process_backlog infra already do a napi_schedule,
thus avoiding the above concern? 

--Sowmini

^ permalink raw reply

* Re: [PATCH net-next 2/2] sunvnet: vnet_start_xmit() must hold a refcnt on port.
From: David Miller @ 2014-10-01 20:24 UTC (permalink / raw)
  To: sowmini.varadhan; +Cc: eric.dumazet, raghuram.kothakota, netdev
In-Reply-To: <542C61DD.8010504@oracle.com>

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Wed, 01 Oct 2014 16:19:41 -0400

> On 10/01/2014 04:15 PM, Eric Dumazet wrote:
>> On Wed, 2014-10-01 at 15:44 -0400, Sowmini Varadhan wrote:
>>> On (10/01/14 12:05), Eric Dumazet wrote:
>>>>
>>>> Hmpff... This calls for rcu protection here !
>>>>
>>>
>>> I did consider that, but given that the lists containing the ports are
>>> accessed in multiple contexts, some of which can sleep, and given that
>>> the vnet port is similar in spirit to the net_device, I followed the
>>> net_device model of dev_put etc.
>>
>> dev_put() uses per cpu variables, an order of magnitude faster than a
>> atomic put/get :(
> 
> I could make this a per-cpu variable, it would not be too hard to
> change
> vnet_put/hold etc. That's not a problem.

If you went with NAPI you could use RCU.

All of the negative and complicated aspects of your changes strictly
have to do with not going with that implementation route.

^ permalink raw reply

* [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context
From: Sowmini Varadhan @ 2014-10-01 20:24 UTC (permalink / raw)
  To: davem, raghuram.kothakota, sowmini.varadhan, david.stevens; +Cc: netdev


The existing sunvnet implementation does all the packet interception
in LDC interrupt context. Patch 1 of this series moves the data
processing to a bottom-half handler.

Some context for the reasons for choosing a BH handler over NAPI:
   A NAPI (or simple tasklet) based implementation provides softirq
   context, which allows the driver to safely invoke netif_receive_skb()
   to deliver the packet to the IP stack. But in the case of sunvnet,
   we are already receiving multiple packets for a single ldc_rx
   interrupt, so the budget-based softirq-vs-polling infra does not
   provide a significant optimization. Rather, it can get in the way,
   if we constrain the vnet-rx path to a poorly chosen budget, and force
   ourselves to send a STOPPED/START ldc exchange needlessly.

   A BH Rx handler is a simpler way to  avoid the weaknesses of processing
   packets in LDC interrupt context, and also provides Rx load-spreading
   across multiple CPUs.

Note that PATCH 1 is dependant on the functions added as part of the
sparc-next commit "sparc64: Add vio_set_intr() to enable/disable Rx interrupts"
(Cf: http://www.spinics.net/lists/sparclinux/msg12647.html)

PATCH 2 of this series fixes a race-condition between vnet_port_remove()
and vnet_start_xmit().

changes since v1 to Patch 2: revert switch_port caching in struct vnet

Sowmini Varadhan (2):
  Process Rx data packets in a BH handler
  vnet_start_xmit() must hold a refcnt on port.

 drivers/net/ethernet/sun/sunvnet.c | 187 +++++++++++++++++++++++++++----------
 drivers/net/ethernet/sun/sunvnet.h |  12 ++-
 2 files changed, 146 insertions(+), 53 deletions(-)

-- 
1.8.4.2

^ permalink raw reply

* [PATCHv2 net-next 1/2] sunvnet: Process Rx data packets in a BH handler
From: Sowmini Varadhan @ 2014-10-01 20:25 UTC (permalink / raw)
  To: davem, raghuram.kothakota, sowmini.varadhan, david.stevens; +Cc: netdev


Move VIO DATA processing out of interrupt context,
and into a bottom-half handler (vnet_event_bh())

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Raghuram Kothakota <raghuram.kothakota@oracle.com>
---
 drivers/net/ethernet/sun/sunvnet.c | 126 ++++++++++++++++++++++++-------------
 drivers/net/ethernet/sun/sunvnet.h |  10 ++-
 2 files changed, 91 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index 1262697..e2aacf5 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -274,6 +274,7 @@ static struct sk_buff *alloc_and_align_skb(struct net_device *dev,
 	return skb;
 }
 
+/* reads in exactly one sk_buff */
 static int vnet_rx_one(struct vnet_port *port, unsigned int len,
 		       struct ldc_trans_cookie *cookies, int ncookies)
 {
@@ -311,9 +312,8 @@ static int vnet_rx_one(struct vnet_port *port, unsigned int len,
 
 	dev->stats.rx_packets++;
 	dev->stats.rx_bytes += len;
-
-	netif_rx(skb);
-
+	/* BH context cannot call netif_receive_skb */
+	netif_rx_ni(skb);
 	return 0;
 
 out_free_skb:
@@ -534,7 +534,10 @@ static int vnet_ack(struct vnet_port *port, void *msgbuf)
 	struct net_device *dev;
 	struct vnet *vp;
 	u32 end;
+	unsigned long flags;
 	struct vio_net_desc *desc;
+	bool need_trigger = false;
+
 	if (unlikely(pkt->tag.stype_env != VIO_DRING_DATA))
 		return 0;
 
@@ -545,21 +548,17 @@ static int vnet_ack(struct vnet_port *port, void *msgbuf)
 	/* sync for race conditions with vnet_start_xmit() and tell xmit it
 	 * is time to send a trigger.
 	 */
+	spin_lock_irqsave(&port->vio.lock, flags);
 	dr->cons = next_idx(end, dr);
 	desc = vio_dring_entry(dr, dr->cons);
-	if (desc->hdr.state == VIO_DESC_READY && port->start_cons) {
-		/* vnet_start_xmit() just populated this dring but missed
-		 * sending the "start" LDC message to the consumer.
-		 * Send a "start" trigger on its behalf.
-		 */
-		if (__vnet_tx_trigger(port, dr->cons) > 0)
-			port->start_cons = false;
-		else
-			port->start_cons = true;
-	} else {
-		port->start_cons = true;
-	}
+	if (desc->hdr.state == VIO_DESC_READY && !port->start_cons)
+		need_trigger = true;
+	else
+		port->start_cons = true; /* vnet_start_xmit will send trigger */
+	spin_unlock_irqrestore(&port->vio.lock, flags);
 
+	if (need_trigger && __vnet_tx_trigger(port, dr->cons) <= 0)
+		port->start_cons = true;
 
 	vp = port->vp;
 	dev = vp->dev;
@@ -617,33 +616,13 @@ static void maybe_tx_wakeup(unsigned long param)
 	netif_tx_unlock(dev);
 }
 
-static void vnet_event(void *arg, int event)
+static void vnet_event_bh(struct work_struct *work)
 {
-	struct vnet_port *port = arg;
+	struct vnet_port *port = container_of(work, struct vnet_port, rx_work);
 	struct vio_driver_state *vio = &port->vio;
-	unsigned long flags;
 	int tx_wakeup, err;
 
-	spin_lock_irqsave(&vio->lock, flags);
-
-	if (unlikely(event == LDC_EVENT_RESET ||
-		     event == LDC_EVENT_UP)) {
-		vio_link_state_change(vio, event);
-		spin_unlock_irqrestore(&vio->lock, flags);
-
-		if (event == LDC_EVENT_RESET) {
-			port->rmtu = 0;
-			vio_port_up(vio);
-		}
-		return;
-	}
-
-	if (unlikely(event != LDC_EVENT_DATA_READY)) {
-		pr_warn("Unexpected LDC event %d\n", event);
-		spin_unlock_irqrestore(&vio->lock, flags);
-		return;
-	}
-
+	mutex_lock(&port->vnet_rx_mutex);
 	tx_wakeup = err = 0;
 	while (1) {
 		union {
@@ -691,14 +670,41 @@ static void vnet_event(void *arg, int event)
 		if (err == -ECONNRESET)
 			break;
 	}
-	spin_unlock(&vio->lock);
-	/* Kick off a tasklet to wake the queue.  We cannot call
-	 * maybe_tx_wakeup directly here because we could deadlock on
-	 * netif_tx_lock() with dev_watchdog()
-	 */
 	if (unlikely(tx_wakeup && err != -ECONNRESET))
 		tasklet_schedule(&port->vp->vnet_tx_wakeup);
+	mutex_unlock(&port->vnet_rx_mutex);
+	vio_set_intr(vio->vdev->rx_ino, HV_INTR_ENABLED);
+}
+
+static void vnet_event(void *arg, int event)
+{
+	struct vnet_port *port = arg;
+	struct vio_driver_state *vio = &port->vio;
+	unsigned long flags;
 
+	spin_lock_irqsave(&vio->lock, flags);
+
+	if (unlikely(event == LDC_EVENT_RESET ||
+		     event == LDC_EVENT_UP)) {
+		vio_link_state_change(vio, event);
+		spin_unlock_irqrestore(&vio->lock, flags);
+
+		if (event == LDC_EVENT_RESET)
+			vio_port_up(vio);
+		return;
+	}
+
+	if (unlikely(event != LDC_EVENT_DATA_READY)) {
+		pr_warn("Unexpected LDC event %d\n", event);
+		spin_unlock_irqrestore(&vio->lock, flags);
+		return;
+	}
+
+	if ((port->flags & VNET_PORT_DEAD) == 0) {
+		vio_set_intr(vio->vdev->rx_ino, HV_INTR_DISABLED);
+		queue_work(port->rx_workq, &port->rx_work);
+	}
+	spin_unlock(&vio->lock);
 	local_irq_restore(flags);
 }
 
@@ -750,6 +756,11 @@ static inline bool port_is_up(struct vnet_port *vnet)
 {
 	struct vio_driver_state *vio = &vnet->vio;
 
+	/* Should never hit a DEAD port here: we are holding the vnet lock,
+	 * and the list cleanup and VNET_PORT_DEAD marking gets done
+	 * under the vnet lock as well.
+	 */
+	BUG_ON(vnet->flags & VNET_PORT_DEAD);
 	return !!(vio->hs_state & VIO_HS_COMPLETE);
 }
 
@@ -1487,6 +1498,23 @@ static void print_version(void)
 	printk_once(KERN_INFO "%s", version);
 }
 
+static int vnet_workq_enable(struct vnet_port *port)
+{
+	port->rx_workq = alloc_workqueue(dev_name(&port->vio.vdev->dev),
+					 WQ_HIGHPRI|WQ_UNBOUND, 1);
+	if (!port->rx_workq)
+		return -ENOMEM;
+	mutex_init(&port->vnet_rx_mutex);
+	INIT_WORK(&port->rx_work, vnet_event_bh);
+	return 0;
+}
+
+static void vnet_workq_disable(struct vnet_port *port)
+{
+	flush_workqueue(port->rx_workq);
+	destroy_workqueue(port->rx_workq);
+}
+
 const char *remote_macaddr_prop = "remote-mac-address";
 
 static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id)
@@ -1536,6 +1564,10 @@ static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 	if (err)
 		goto err_out_free_port;
 
+	err = vnet_workq_enable(port);
+	if (err)
+		goto err_out_free_port;
+
 	err = vnet_port_alloc_tx_bufs(port);
 	if (err)
 		goto err_out_free_ldc;
@@ -1572,6 +1604,7 @@ static int vnet_port_probe(struct vio_dev *vdev, const struct vio_device_id *id)
 
 err_out_free_ldc:
 	vio_ldc_free(&port->vio);
+	destroy_workqueue(port->rx_workq);
 
 err_out_free_port:
 	kfree(port);
@@ -1589,14 +1622,21 @@ static int vnet_port_remove(struct vio_dev *vdev)
 		struct vnet *vp = port->vp;
 		unsigned long flags;
 
+		vio_set_intr(port->vio.vdev->rx_ino, HV_INTR_DISABLED);
 		del_timer_sync(&port->vio.timer);
 		del_timer_sync(&port->clean_timer);
 
+		/* VNET_PORT_DEAD disallows any more vnet_event_bh
+		 * scheduling and prevents new refs to the port
+		 */
 		spin_lock_irqsave(&vp->lock, flags);
+		port->flags |= VNET_PORT_DEAD;
 		list_del(&port->list);
 		hlist_del(&port->hash);
 		spin_unlock_irqrestore(&vp->lock, flags);
 
+		vnet_workq_disable(port);
+
 		vnet_port_free_tx_bufs(port);
 		vio_ldc_free(&port->vio);
 
diff --git a/drivers/net/ethernet/sun/sunvnet.h b/drivers/net/ethernet/sun/sunvnet.h
index c911045..1182ec6 100644
--- a/drivers/net/ethernet/sun/sunvnet.h
+++ b/drivers/net/ethernet/sun/sunvnet.h
@@ -2,7 +2,7 @@
 #define _SUNVNET_H
 
 #include <linux/interrupt.h>
-
+#include <linux/workqueue.h>
 #define DESC_NCOOKIES(entry_size)	\
 	((entry_size) - sizeof(struct vio_net_desc))
 
@@ -41,7 +41,8 @@ struct vnet_port {
 	struct hlist_node	hash;
 	u8			raddr[ETH_ALEN];
 	u8			switch_port;
-	u8			__pad;
+	u8			flags;
+#define	VNET_PORT_DEAD	0x01
 
 	struct vnet		*vp;
 
@@ -56,6 +57,11 @@ struct vnet_port {
 	struct timer_list	clean_timer;
 
 	u64			rmtu;
+
+	struct mutex            vnet_rx_mutex; /* serializes rx_workq */
+	struct work_struct      rx_work;
+	struct workqueue_struct *rx_workq;
+
 };
 
 static inline struct vnet_port *to_vnet_port(struct vio_driver_state *vio)
-- 
1.8.4.2

^ permalink raw reply related

* [PATCH net-next 2/2] vnet_start_xmit() must hold a refcnt on port.
From: Sowmini Varadhan @ 2014-10-01 20:25 UTC (permalink / raw)
  To: davem, raghuram.kothakota, sowmini.varadhan, david.stevens; +Cc: netdev


A vnet_port_remove could be triggered as a result of an ldm-unbind
operation
by the peer, module unload, or other changes to the inter-vnet-link
configuration.  When this is concurrent with vnet_start_xmit(),
the following sequence could occur

  thread 1                                    thread 2
vnet_start_xmit
 -> tx_port_find
    spin_lock_irqsave(&vp->lock..)
    ret = __tx_port_find(..)
    spin_lock_irqrestore(&vp->lock..)
                                           vio_remove -> ..
                                               ->vnet_port_remove
                                           spin_lock_irqsave(&vp->lock..)
                                           cleanup
                                           spin_lock_irqrestore(&vp->lock..)
                                           kfree(port)
 /* attempt to use ret will bomb */

This patch avoids the problem by holding/releasing a refcnt on
the vnet_port.

Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Raghuram Kothakota <raghuram.kothakota@oracle.com>
---
changes since v1: reverted switch_port caching in `struct vnet'.

 drivers/net/ethernet/sun/sunvnet.c | 46 +++++++++++++++++++++++++++++++++++++-
 drivers/net/ethernet/sun/sunvnet.h |  1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index e2aacf5..62264cb 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -47,6 +47,42 @@ MODULE_VERSION(DRV_MODULE_VERSION);
 
 static int __vnet_tx_trigger(struct vnet_port *port, u32 start);
 
+static inline int vnet_refcnt_read(const struct vnet_port *port)
+{
+	return atomic_read(&port->refcnt);
+}
+
+static inline void vnet_hold(struct vnet_port *port)
+{
+	atomic_inc(&port->refcnt);
+	BUG_ON(vnet_refcnt_read(port) == 0);
+}
+
+static void vnet_put(struct vnet_port *port)
+{
+	atomic_dec(&port->refcnt);
+}
+
+static void vnet_wait_allrefs(struct vnet_port *port)
+{
+	unsigned long warning_time;
+	int refcnt = vnet_refcnt_read(port);
+
+	warning_time = jiffies;
+	while (refcnt != 0) {
+		msleep(250);
+		refcnt = vnet_refcnt_read(port);
+		if (time_after(jiffies, warning_time + 10 * HZ)) {
+			pr_emerg("vnet_wait_allrefs: waiting for port to "
+				 "%x:%x:%x:%x:%x:%x. Usage count = %d\n",
+				port->raddr[0], port->raddr[1],
+				port->raddr[2], port->raddr[3],
+				port->raddr[4], port->raddr[5], refcnt);
+			warning_time = jiffies;
+		}
+	}
+}
+
 /* Ordered from largest major to lowest */
 static struct vio_version vnet_versions[] = {
 	{ .major = 1, .minor = 6 },
@@ -773,14 +809,17 @@ struct vnet_port *__tx_port_find(struct vnet *vp, struct sk_buff *skb)
 	hlist_for_each_entry(port, hp, hash) {
 		if (!port_is_up(port))
 			continue;
-		if (ether_addr_equal(port->raddr, skb->data))
+		if (ether_addr_equal(port->raddr, skb->data)) {
+			vnet_hold(port);
 			return port;
+		}
 	}
 	list_for_each_entry(port, &vp->port_list, list) {
 		if (!port->switch_port)
 			continue;
 		if (!port_is_up(port))
 			continue;
+		vnet_hold(port);
 		return port;
 	}
 	return NULL;
@@ -974,6 +1013,7 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 			dev->stats.tx_errors++;
 		}
 		spin_unlock_irqrestore(&port->vio.lock, flags);
+		vnet_put(port);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -1071,6 +1111,7 @@ ldc_start_done:
 	vnet_free_skbs(freeskbs);
 
 	(void)mod_timer(&port->clean_timer, jiffies + VNET_CLEAN_TIMEOUT);
+	vnet_put(port);
 
 	return NETDEV_TX_OK;
 
@@ -1087,6 +1128,8 @@ out_dropped:
 	else
 		del_timer(&port->clean_timer);
 	dev->stats.tx_dropped++;
+	if (port)
+		vnet_put(port);
 	return NETDEV_TX_OK;
 }
 
@@ -1636,6 +1679,7 @@ static int vnet_port_remove(struct vio_dev *vdev)
 		spin_unlock_irqrestore(&vp->lock, flags);
 
 		vnet_workq_disable(port);
+		vnet_wait_allrefs(port);
 
 		vnet_port_free_tx_bufs(port);
 		vio_ldc_free(&port->vio);
diff --git a/drivers/net/ethernet/sun/sunvnet.h b/drivers/net/ethernet/sun/sunvnet.h
index 1182ec6..5c35474 100644
--- a/drivers/net/ethernet/sun/sunvnet.h
+++ b/drivers/net/ethernet/sun/sunvnet.h
@@ -53,6 +53,7 @@ struct vnet_port {
 	u32			stop_rx_idx;
 	bool			stop_rx;
 	bool			start_cons;
+	atomic_t		refcnt;
 
 	struct timer_list	clean_timer;
 
-- 
1.8.4.2

^ permalink raw reply related

* Re: [PATCH net-next 0/2] sunvnet: Packet processing in non-interrupt context.
From: David Miller @ 2014-10-01 20:25 UTC (permalink / raw)
  To: sowmini.varadhan; +Cc: raghuram.kothakota, netdev
In-Reply-To: <20141001202315.GN17706@oracle.com>

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Wed, 1 Oct 2014 16:23:15 -0400

> On (10/01/14 16:15), David Miller wrote:
>> > 
>> > If I make this a NAPI driver that uses napi_gro_receive, I would
>> > still have to deal with a budget, right?
>> 
>> Absolutely, and YOU MUST, because the budget keeps one device from
>> hogging the RX packet input path from other devices on a given cpu.
> 
> yes, but limiting the budget of sk_buffs read mid-way during descriptor
> read is deadly to perf because of the ensuing LDC stop/start exchange -
> ends up being even worse than the baseline.
> 
> Doesnt the netif_rx/process_backlog infra already do a napi_schedule,
> thus avoiding the above concern? 

The limit is by default 64 packets, it won't matter.

I think you're overplaying the things that block use of NAPI, how
about implementing it properly, using netif_gso_receive() and proper
RCU accesses, and coming back with some real performance measurements?

^ permalink raw reply

* Re: [PATCH net-next v2] tcp: abort orphan sockets stalling on zero window probes
From: David Miller @ 2014-10-01 20:28 UTC (permalink / raw)
  To: ycheng; +Cc: edumazet, andrey.dmitrov, ncardwell, netdev
In-Reply-To: <1412022038-14408-1-git-send-email-ycheng@google.com>

From: Yuchung Cheng <ycheng@google.com>
Date: Mon, 29 Sep 2014 13:20:38 -0700

> Currently we have two different policies for orphan sockets
> that repeatedly stall on zero window ACKs. If a socket gets
> a zero window ACK when it is transmitting data, the RTO is
> used to probe the window. The socket is aborted after roughly
> tcp_orphan_retries() retries (as in tcp_write_timeout()).
> 
> But if the socket was idle when it received the zero window ACK,
> and later wants to send more data, we use the probe timer to
> probe the window. If the receiver always returns zero window ACKs,
> icsk_probes keeps getting reset in tcp_ack() and the orphan socket
> can stall forever until the system reaches the orphan limit (as
> commented in tcp_probe_timer()). This opens up a simple attack
> to create lots of hanging orphan sockets to burn the memory
> and the CPU, as demonstrated in the recent netdev post "TCP
> connection will hang in FIN_WAIT1 after closing if zero window is
> advertised." http://www.spinics.net/lists/netdev/msg296539.html
> 
> This patch follows the design in RTO-based probe: we abort an orphan
> socket stalling on zero window when the probe timer reaches both
> the maximum backoff and the maximum RTO. For example, an 100ms RTT
> connection will timeout after roughly 153 seconds (0.3 + 0.6 +
> .... + 76.8) if the receiver keeps the window shut. If the orphan
> socket passes this check, but the system already has too many orphans
> (as in tcp_out_of_resources()), we still abort it but we'll also
> send an RST packet as the connection may still be active.
> 
> In addition, we change TCP_USER_TIMEOUT to cover (life or dead)
> sockets stalled on zero-window probes. This changes the semantics
> of TCP_USER_TIMEOUT slightly because it previously only applies
> when the socket has pending transmission.
> 
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>

Applied, thanks a lot.

^ permalink raw reply

* Re: [net-next PATCH V5] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jesper Dangaard Brouer @ 2014-10-01 20:32 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: Tom Herbert, David Miller, Linux Netdev List, Eric Dumazet,
	Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
	Alexander Duyck, John Fastabend, Dave Taht,
	Toke Høiland-Jørgensen, brouer
In-Reply-To: <542C5E8B.7070204@mojatatu.com>

On Wed, 01 Oct 2014 16:05:31 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> On 10/01/14 15:47, Jesper Dangaard Brouer wrote:
> 
> >
> > Answer is yes.  It is very easy with simple netperf TCP_STREAM to cause
> > queueing >1 packet in the qdisc layer.
> 
> If that is the case, I withdraw any doubts i had.
> Can you please specify this in your commit logs for patch 0?

I'll try to make it more explicit.
Will resubmit patchset shortly...

Notice it is not difficult cause a queue to form, but it is tricky (not
difficult) to correctly test this patchset.  Perhaps you misread my
statement earlier as "it was difficult to test and cause a queue to form"?


> > If tuned (according to my blog,
> > unloading netfilter etc.) then a single netperf TCP_STREAM will max out
> > 10Gbit/s and cause a standing queue.
> >
> 
> You should describe such tuning in the patch log (hard to read
> blogs for more than 30 seconds; write a paper if you want to provide
> more details).

I think you could read this blog in 30 sec:
 http://netoptimizer.blogspot.dk/2014/04/basic-tuning-for-network-overload.html

My cover letter and testing section... will take you longer that 30
sec, it have grown quite large (and Eric will not even read it :-P ;-))

Believe or not, I've actually restricted and reduced the testing
section.  If you want the hole verbose version of my testing for the
upcoming V6 patch, look at this:

 http://people.netfilter.org/hawk/qdisc/measure12_internal_V6_patch/
 http://people.netfilter.org/hawk/qdisc/measure13_V6_patch_NObulk/

And use netperf-wrapper to dive into the data.
A quick setup guide:
 http://netoptimizer.blogspot.dk/2014/09/mini-tutorial-for-netperf-wrapper-setup.html


> > I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never
> > saw a standing queue of 1 packet in my testing.  Either the backlog
> > area is high 100-200 packets, or 0 backlog.  (With fake pktgen/trafgen
> > style tests, it's possible to cause 1000 backlog).
> 
> It would be nice to actually collect such stats. Monitoring the backlog
> via dumping qdisc stats is a good start - but actually keeping traces
> of average bulk size is more useful.

I usually also monitors the BQL limits during these tests.

 grep -H . /sys/class/net/eth4/queues/tx-*/byte_queue_limits/{inflight,limit}

To Toke:
 Perhaps we could convince Toke, to add a netperf-wrapper recorder for
the BQL inflight and limit?  (It would be really cool to plot together)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply

* Re: [PATCH v2 net-next] net: cleanup and document skb fclone layout
From: David Miller @ 2014-10-01 20:34 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev
In-Reply-To: <1412022555.30721.40.camel@edumazet-glaptop2.roam.corp.google.com>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 29 Sep 2014 13:29:15 -0700

> 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,
> to stop leaking of implementation details.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: rebased on latest net-next

Applied, thank you.

^ permalink raw reply

* [net-next PATCH V6 0/2] qdisc: bulk dequeue support
From: Jesper Dangaard Brouer @ 2014-10-01 20:35 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, netdev, David S. Miller, Tom Herbert,
	Eric Dumazet, Hannes Frederic Sowa, Florian Westphal,
	Daniel Borkmann
  Cc: Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
	toke

This patchset uses DaveM's recent API changes to dev_hard_start_xmit(),
from the qdisc layer, to implement dequeue bulking.

Patch01: "qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE"
 - Implement basic qdisc dequeue bulking
 - This time, 100% relying on BQL limits, no magic safe-guard constants

Patch02: "qdisc: dequeue bulking also pickup GSO/TSO packets"
 - Extend bulking to bulk several GSO/TSO packets
 - Seperate patch, as it introduce a small regression, see test section.

We do have a patch03, which exports a userspace tunable as a BQL
tunable, that can byte-cap or disable the bulking/bursting.  But we
could not agree on it internally, thus not sending it now.  We
basically strive to avoid adding any new userspace tunable.


Testing patch01:
================
 Demonstrating the performance improvement of qdisc dequeue bulking, is
tricky because the effect only "kicks-in" once the qdisc system have a
backlog. Thus, for a backlog to form, we need either 1) to exceed wirespeed
of the link or 2) exceed the capability of the device driver.

For practical use-cases, the measureable effect of this will be a
reduction in CPU usage

01-TCP_STREAM:
--------------
Testing effect for TCP involves disabling TSO and GSO, because TCP
already benefit from bulking, via TSO and especially for GSO segmented
packets.  This patch view TSO/GSO as a seperate kind of bulking, and
avoid further bulking of these packet types.

The measured perf diff benefit (at 10Gbit/s) for a single netperf
TCP_STREAM were 9.24% less CPU used on calls to _raw_spin_lock()
(mostly from sch_direct_xmit).

If my E5-2695v2(ES) CPU is tuned according to:
 http://netoptimizer.blogspot.dk/2014/04/basic-tuning-for-network-overload.html
Then it is possible that a single netperf TCP_STREAM, with GSO and TSO
disabled, can utilize all bandwidth on a 10Gbit/s link.  This will
then cause a standing backlog queue at the qdisc layer.

Trying to pressure the system some more CPU util wise, I'm starting
24x TCP_STREAMs and monitoring the overall CPU utilization.  This
confirms bulking saves CPU cycles when it "kicks-in".

Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
 * Disabled bulking: sys:2.58%  soft:8.50%  idle:88.78%
 * Enabled  bulking: sys:2.43%  soft:7.66%  idle:89.79%

02-UDP_STREAM
-------------
The measured perf diff benefit for UDP_STREAM were 6.41% less CPU used
on calls to _raw_spin_lock().  24x UDP_STREAM with packet size -m 1472 (to
avoid sending UDP/IP fragments).

03-trafgen driver test
----------------------
The performance of the 10Gbit/s ixgbe driver is limited due to
updating the HW ring-queue tail-pointer on every packet.  As
previously demonstrated with pktgen.

Using trafgen to send RAW frames from userspace (via AF_PACKET), and
forcing it through qdisc path (with option --qdisc-path and -t0),
sending with 12 CPUs.

I can demonstrate this driver layer limitation:
 * 12.8 Mpps with no qdisc bulking
 * 14.8 Mpps with qdisc bulking (full 10G-wirespeed)


Testing patch02:
================
Testing Bulking several GSO/TSO packets:

Measuring HoL (Head-of-Line) blocking for TSO and GSO, with
netperf-wrapper. Bulking several TSO show no performance regressions
(requeues were in the area 32 requeues/sec for 10G while transmitting
approx 813Kpps).

Bulking several GSOs does show small regression or very small
improvement (requeues were in the area 8000 requeues/sec, for 10G
while transmitting approx 813Kpps).

 Using ixgbe 10Gbit/s with GSO bulking, we can measure some additional
latency. Base-case, which is "normal" GSO bulking, sees varying
high-prio queue delay between 0.38ms to 0.47ms.  Bulking several GSOs
together, result in a stable high-prio queue delay of 0.50ms.

Corrosponding to:
 (10000*10^6)*((0.50-0.47)/10^3)/8 = 37500 bytes
 (10000*10^6)*((0.50-0.38)/10^3)/8 = 150000 bytes
 37500/1500  = 25 pkts
 150000/1500 = 100 pkts

 Using igb at 100Mbit/s with GSO bulking, shows an improvement.
Base-case sees varying high-prio queue delay between 2.23ms to 2.35ms
diff of 0.12ms corrosponding to 1500 bytes at 100Mbit/s. Bulking
several GSOs together, result in a stable high-prio queue delay of
2.23ms.

---

Jesper Dangaard Brouer (2):
      qdisc: dequeue bulking also pickup GSO/TSO packets
      qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE


 include/net/sch_generic.h |   16 ++++++++++++++++
 net/sched/sch_generic.c   |   40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 2 deletions(-)

-- 

^ permalink raw reply

* Re: [PATCH V3 next] r8169: add support for Byte Queue Limits
From: David Miller @ 2014-10-01 20:36 UTC (permalink / raw)
  To: fw; +Cc: netdev, romieu, hayeswang
In-Reply-To: <1412163483-24560-1-git-send-email-fw@strlen.de>

From: Florian Westphal <fw@strlen.de>
Date: Wed,  1 Oct 2014 13:38:03 +0200

> tested on RTL8168d/8111d model using 'super_netperf 40' with TCP/UDP_STREAM.
> 
> Output of
> while true; do
>     for n in inflight limit; do
>           echo -n $n\ ; cat $n;
>     done;
>     sleep 1;
> done
 ...
> [end of test]
> 
> Cc: Francois Romieu <romieu@fr.zoreil.com>
> Cc: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Acked-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Tom Herbert <therbert@google.com>
> ---
>  Change since v2: updated commit message.

Applied, thank you.

^ permalink raw reply


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