low latency/busy poll feedback and bugs

low latency/busy poll feedback and bugs

[Shawn Bohrer]

I did a little testing of the new low latency/busy poll sockets today
and found a few things that surprised me and at least one bug.

1) I'm testing with a Mellanox ConnectX-3 card.  Currently polling
with mlx4_en is broken when GRO is enabled.  In
mlx4_en_process_rx_cq() when GRO is enabled skb_mark_napi_id() is
never called.  It appears like low latency sockets with GRO is
supposed to work because the following code checks that we are not
ll_polling:

 /* This packet is eligible for GRO if it is:
   * - DIX Ethernet (type interpretation)
   * - TCP/IP (v4)
   * - without IP options
   * - not an IP fragment
   * - no LLS polling in progress
   */
  if (!mlx4_en_cq_ll_polling(cq) &&
      (dev->features & NETIF_F_GRO)) {

However since we never call skb_mark_napi_id() mlx4_en_cq_ll_polling()
will never be true.

2) Why is LowLatencyRxPackets reported as a TcpExt stat?  Perhaps I've
been confused and misguided but I've always assumed those are
statistics related to TCP and this feature is protocol neutral.  I'm
not entirely sure where it should be moved to perhaps IpExt?

3) I don't know if this was intentional, an oversight, or simply a
missing feature but UDP multicast currently is not supported.  In
order to add support I believe you would need to call
sk_mark_napi_id() in __udp4_lib_mcast_deliver().  Assuming there isn't
some intentional reason this wasn't done I'd be happy to test this and
send a patch.

--
Shawn

-- 

---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.

[PATCH net-next] net: Add low-latency/polling support for UDP multicast

[Shawn Bohrer]

Set the napi id for each socket in the multicast path to enable
low-latency/polling support.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
 net/ipv4/udp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 766e6ba..0d0da17 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1596,6 +1596,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	dif = skb->dev->ifindex;
 	sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
 	while (sk) {
+		sk_mark_napi_id(sk, skb);
 		stack[count++] = sk;
 		sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
 				       daddr, uh->source, saddr, dif);
-- 
1.7.7.6


-- 

---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.

Re: [PATCH net-next] net: Add low-latency/polling support for UDP multicast

[Eliezer Tamir]

On 06/08/2013 01:16, Shawn Bohrer wrote:
> Set the napi id for each socket in the multicast path to enable
> low-latency/polling support.
> 
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
>  net/ipv4/udp.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 766e6ba..0d0da17 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1596,6 +1596,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	dif = skb->dev->ifindex;
>  	sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
>  	while (sk) {
> +		sk_mark_napi_id(sk, skb);
>  		stack[count++] = sk;
>  		sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
>  				       daddr, uh->source, saddr, dif);
> 

Looks OK.
To be complete, you want to do the same for __udp6_lib_mcast_deliver().

-Eliezer

Re: low latency/busy poll feedback and bugs

[Eliezer Tamir]

On 06/08/2013 00:22, Shawn Bohrer wrote:
> 1) I'm testing with a Mellanox ConnectX-3 card.

There's your problem ;)

> 2) Why is LowLatencyRxPackets reported as a TcpExt stat?  Perhaps I've
> been confused and misguided but I've always assumed those are
> statistics related to TCP and this feature is protocol neutral.  I'm
> not entirely sure where it should be moved to perhaps IpExt?

Actually, after all of the rewrite this has gone through,
it's now at the Ethernet level, not even IP specific.

So where should it go?

Should we also rename this to BusyPollRxPackets?

> 3) I don't know if this was intentional, an oversight, or simply a
> missing feature but UDP multicast currently is not supported.  In
> order to add support I believe you would need to call
> sk_mark_napi_id() in __udp4_lib_mcast_deliver().  Assuming there isn't
> some intentional reason this wasn't done I'd be happy to test this and
> send a patch.

This is still WIP, so our goal was to make it easy to extend for new
cases and protocols.

For multicast, it is possible that incoming packets to come from more
than one port (and therefore more than one queue).
I'm not sure how we could handle that, but what we have today won't do
well for that use-case.

What do you use for testing?

-Eliezer

Re: low latency/busy poll feedback and bugs

[Amir Vadai]

On 06/08/2013 00:22, Shawn Bohrer wrote:
> I did a little testing of the new low latency/busy poll sockets today
> and found a few things that surprised me and at least one bug.
> 
> 1) I'm testing with a Mellanox ConnectX-3 card.  
Of course :)

> Currently polling
> with mlx4_en is broken when GRO is enabled.  In
> mlx4_en_process_rx_cq() when GRO is enabled skb_mark_napi_id() is
> never called.  It appears like low latency sockets with GRO is
> supposed to work because the following code checks that we are not
> ll_polling:
> 
>  /* This packet is eligible for GRO if it is:
>    * - DIX Ethernet (type interpretation)
>    * - TCP/IP (v4)
>    * - without IP options
>    * - not an IP fragment
>    * - no LLS polling in progress
>    */
>   if (!mlx4_en_cq_ll_polling(cq) &&
>       (dev->features & NETIF_F_GRO)) {
> 
> However since we never call skb_mark_napi_id() mlx4_en_cq_ll_polling()
> will never be true.

Currently GRO and LLS are mutually exclusive, and shouldn't be used
together.
I will send a fix to make the code clearer soon.

Thanks for reporting,
Amir

Re: low latency/busy poll feedback and bugs

[Shawn Bohrer]

On Tue, Aug 06, 2013 at 10:41:48AM +0300, Eliezer Tamir wrote:
> On 06/08/2013 00:22, Shawn Bohrer wrote:
> > 3) I don't know if this was intentional, an oversight, or simply a
> > missing feature but UDP multicast currently is not supported.  In
> > order to add support I believe you would need to call
> > sk_mark_napi_id() in __udp4_lib_mcast_deliver().  Assuming there isn't
> > some intentional reason this wasn't done I'd be happy to test this and
> > send a patch.
> 
> This is still WIP, so our goal was to make it easy to extend for new
> cases and protocols.
> 
> For multicast, it is possible that incoming packets to come from more
> than one port (and therefore more than one queue).
> I'm not sure how we could handle that, but what we have today won't do
> well for that use-case.
 
It is unclear to me exactly what happens in this case.  With my simple
patch I'm assuming it will spin on the receive queue that received the
last packet for that socket.  What happens when a packet arrives on a
different receive queue than the one we were spinning on? I assume it
is still delivered but perhaps the spinning process won't get it until
the spinning time expires?  I'm just guessing and haven't attempted to
figure it out from looking through the code.

I put together a small test case with two senders and a single
receiver, and visually (by watching /proc/interrups) verified that
their traffic went to two different queues.  The receiver received all
of the packets with busy_read enabled so it appears that it at least
superficially works.  I did not verify the effect on latency.

> What do you use for testing?

In fio 2.1.2 [1] I added support for UDP multicast.  It's not quite as
flexible as I would like but you can still test a number of scenarios
like the one above or do a basic pingpong test.

Here are my fio job files for a pingpong test:

$ cat mcudp_rr_receive
[global]
ioengine=net
protocol=udp
bs=64
size=100m
# IP address of interface to receive packets
#interface=10.8.16.21
rw=read

[pingpong]
pingpong=1
port=10000
hostname=239.0.0.0

$ cat mcudp_rr_send
[global]
ioengine=net
protocol=udp
bs=64
size=100m
# IP address of interface to send packets
#interface=10.8.16.22
rw=write

[pingpong]
pingpong=1
port=10000
hostname=239.0.0.0

Just start the receiver on one host then start the sender on a second
host:

[host1] $ fio mcudp_rr_receive

[host2] $ fio mcudp_rr_send

[1] http://brick.kernel.dk/snaps/fio-2.1.2.tar.bz2

--
Shawn

-- 

---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.

Re: low latency/busy poll feedback and bugs

[Eliezer Tamir]

On 06/08/2013 21:08, Shawn Bohrer wrote:
> On Tue, Aug 06, 2013 at 10:41:48AM +0300, Eliezer Tamir wrote:
>> For multicast, it is possible that incoming packets to come from more
>> than one port (and therefore more than one queue).
>> I'm not sure how we could handle that, but what we have today won't do
>> well for that use-case.
>  
> It is unclear to me exactly what happens in this case.  With my simple
> patch I'm assuming it will spin on the receive queue that received the
> last packet for that socket.  What happens when a packet arrives on a
> different receive queue than the one we were spinning on? I assume it
> is still delivered but perhaps the spinning process won't get it until
> the spinning time expires?  I'm just guessing and haven't attempted to
> figure it out from looking through the code.

What will happen is that the current code will only busy poll on one
queue, sometimes on this one, sometimes on that one.

packets arriving on the other queue will still be serviced but will
suffer the latency of waiting for NAPI to schedule.

So your avg will be better, but your std. dev. much worse and it's
probably not worth it if you really expect two devices to receive
data at the same time.

[PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast

[Shawn Bohrer]

Set the napi id for each socket in the multicast path to enable
low-latency/polling support.

Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
---
v2 include ipv6 support

 net/ipv4/udp.c |    1 +
 net/ipv6/udp.c |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 766e6ba..0d0da17 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1596,6 +1596,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	dif = skb->dev->ifindex;
 	sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
 	while (sk) {
+		sk_mark_napi_id(sk, skb);
 		stack[count++] = sk;
 		sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
 				       daddr, uh->source, saddr, dif);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index f405815..82be372 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -756,6 +756,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	dif = inet6_iif(skb);
 	sk = udp_v6_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
 	while (sk) {
+		sk_mark_napi_id(sk, skb);
 		stack[count++] = sk;
 		sk = udp_v6_mcast_next(net, sk_nulls_next(sk), uh->dest, daddr,
 				       uh->source, saddr, dif);
-- 
1.7.7.6


-- 

---------------------------------------------------------------
This email, along with any attachments, is confidential. If you 
believe you received this message in error, please contact the 
sender immediately and delete all copies of the message.  
Thank you.

Re: low latency/busy poll feedback and bugs

[Or Gerlitz]

On Tue, Aug 6, 2013 at 10:41 AM, Eliezer Tamir
<eliezer.tamir@linux.intel.com> wrote:
> On 06/08/2013 00:22, Shawn Bohrer wrote:
>> 1) I'm testing with a Mellanox ConnectX-3 card.
>
> There's your problem ;)

Can you be more specific? I love testing with Intel Niantic, do code
inspetion of the ixgbe driver and interact with Jeff/Greg/Alex/John
and all the other great folks from the Intel Linux networking team,
actually that (happily working with your competitors) is crucial part
of the fun in open source / upstream work. What's wrong with using
NICs which don't have intel.com on their paper cover?

Or.

Re: low latency/busy poll feedback and bugs

[Eric Dumazet]

On Tue, 2013-08-06 at 23:39 +0300, Or Gerlitz wrote:
> On Tue, Aug 6, 2013 at 10:41 AM, Eliezer Tamir
> <eliezer.tamir@linux.intel.com> wrote:
> > On 06/08/2013 00:22, Shawn Bohrer wrote:
> >> 1) I'm testing with a Mellanox ConnectX-3 card.
> >
> > There's your problem ;)
> 
> Can you be more specific? I love testing with Intel Niantic, do code
> inspetion of the ixgbe driver and interact with Jeff/Greg/Alex/John
> and all the other great folks from the Intel Linux networking team,
> actually that (happily working with your competitors) is crucial part
> of the fun in open source / upstream work. What's wrong with using
> NICs which don't have intel.com on their paper cover?

I don't think there is something wrong.

Eliezer tone was quite clear ;)

BTW, I do not have a Niantic cards to play with, Intel guys feel free to
send me samples ;)

Thanks

Re: low latency/busy poll feedback and bugs

[Ben Hutchings]

On Tue, 2013-08-06 at 21:25 +0300, Eliezer Tamir wrote:
> On 06/08/2013 21:08, Shawn Bohrer wrote:
> > On Tue, Aug 06, 2013 at 10:41:48AM +0300, Eliezer Tamir wrote:
> >> For multicast, it is possible that incoming packets to come from more
> >> than one port (and therefore more than one queue).
> >> I'm not sure how we could handle that, but what we have today won't do
> >> well for that use-case.
> >  
> > It is unclear to me exactly what happens in this case.  With my simple
> > patch I'm assuming it will spin on the receive queue that received the
> > last packet for that socket.  What happens when a packet arrives on a
> > different receive queue than the one we were spinning on? I assume it
> > is still delivered but perhaps the spinning process won't get it until
> > the spinning time expires?  I'm just guessing and haven't attempted to
> > figure it out from looking through the code.
> 
> What will happen is that the current code will only busy poll on one
> queue, sometimes on this one, sometimes on that one.
> 
> packets arriving on the other queue will still be serviced but will
> suffer the latency of waiting for NAPI to schedule.
> 
> So your avg will be better, but your std. dev. much worse and it's
> probably not worth it if you really expect two devices to receive
> data at the same time.

It seems like sk_mark_napi_id() should only be called on connected
sockets for now.

At least Solarflare controllers have 'wildcard' filters that can match
destination address (host, l4proto, port) only.  Perhaps ARFS could be
extended to include steering based on destination address when there is
a single unconnected socket bound to that address.  When that is
successful, busy- polling a single NAPI context should work nicely.

Where there are multiple unconnected sockets bound to the same unicast
address, it might make sense for polling on those sockets to prefer the
'local' NAPI context according to CPU topology.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast

[Eric Dumazet]

On Tue, 2013-08-06 at 14:51 -0500, Shawn Bohrer wrote:
> Set the napi id for each socket in the multicast path to enable
> low-latency/polling support.
> 
> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> ---
> v2 include ipv6 support

This might help your workload, but I doubt it is generic enough.

One UDP socket is supposed to receive traffic from many endpoints,
so we have no guarantee all the received traffic will end on a single RX
queue on the NIC.

That's the same logic than RFS here.

sk_mark_napi_id() in UDP are wrong IMHO.

It should be guarded by the following test in  
__udp_queue_rcv_skb()

if (inet_sk(sk)->inet_daddr) {
    sock_rps_save_rxhash(sk, skb);
    sk_mark_napi_id(sk, skb);
}

(To occur only for connected UDP sockets, where we are 100% sure all
packets will use this same rxhash/rx queue)

Re: low latency/busy poll feedback and bugs

[Eric Dumazet]

On Wed, 2013-08-07 at 22:05 +0200, Ben Hutchings wrote:

> It seems like sk_mark_napi_id() should only be called on connected
> sockets for now.

I concur.

Re: low latency/busy poll feedback and bugs

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 07 Aug 2013 13:23:25 -0700

> On Wed, 2013-08-07 at 22:05 +0200, Ben Hutchings wrote:
> 
>> It seems like sk_mark_napi_id() should only be called on connected
>> sockets for now.
> 
> I concur.

Me too.

Re: [PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast

[Eliezer Tamir]

On 07/08/2013 23:22, Eric Dumazet wrote:
> On Tue, 2013-08-06 at 14:51 -0500, Shawn Bohrer wrote:
>> Set the napi id for each socket in the multicast path to enable
>> low-latency/polling support.
>>
>> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>> ---
>> v2 include ipv6 support
> 
> This might help your workload, but I doubt it is generic enough.
> 
> One UDP socket is supposed to receive traffic from many endpoints,
> so we have no guarantee all the received traffic will end on a single RX
> queue on the NIC.
> 
> That's the same logic than RFS here.
> 
> sk_mark_napi_id() in UDP are wrong IMHO.
> 
> It should be guarded by the following test in  
> __udp_queue_rcv_skb()
> 
> if (inet_sk(sk)->inet_daddr) {
>     sock_rps_save_rxhash(sk, skb);
>     sk_mark_napi_id(sk, skb);
> }
> 
> (To occur only for connected UDP sockets, where we are 100% sure all
> packets will use this same rxhash/rx queue)

This would also be safe if there is only one NIC and said NIC was
programmed to always place this socket's data on the same queue.

I don't have a good suggestion on how to detect this.

Re: [PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast

[Eric Dumazet]

On Thu, 2013-08-08 at 11:46 +0300, Eliezer Tamir wrote:
> On 07/08/2013 23:22, Eric Dumazet wrote:
> > On Tue, 2013-08-06 at 14:51 -0500, Shawn Bohrer wrote:
> >> Set the napi id for each socket in the multicast path to enable
> >> low-latency/polling support.
> >>
> >> Signed-off-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
> >> ---
> >> v2 include ipv6 support
> > 
> > This might help your workload, but I doubt it is generic enough.
> > 
> > One UDP socket is supposed to receive traffic from many endpoints,
> > so we have no guarantee all the received traffic will end on a single RX
> > queue on the NIC.
> > 
> > That's the same logic than RFS here.
> > 
> > sk_mark_napi_id() in UDP are wrong IMHO.
> > 
> > It should be guarded by the following test in  
> > __udp_queue_rcv_skb()
> > 
> > if (inet_sk(sk)->inet_daddr) {
> >     sock_rps_save_rxhash(sk, skb);
> >     sk_mark_napi_id(sk, skb);
> > }
> > 
> > (To occur only for connected UDP sockets, where we are 100% sure all
> > packets will use this same rxhash/rx queue)
> 
> This would also be safe if there is only one NIC and said NIC was
> programmed to always place this socket's data on the same queue.
> 
> I don't have a good suggestion on how to detect this.

Well, this stuff relies on flows being correctly steered.

TCP stack performs much better if we avoid reorders ;)

Re: [PATCH v2 net-next] net: Add low-latency/polling support for UDP multicast

[Eliezer Tamir]

On 09/08/2013 02:55, Eric Dumazet wrote:
> On Thu, 2013-08-08 at 11:46 +0300, Eliezer Tamir wrote:
>> On 07/08/2013 23:22, Eric Dumazet wrote:
>>> sk_mark_napi_id() in UDP are wrong IMHO.
>>>
>>> It should be guarded by the following test in  
>>> __udp_queue_rcv_skb()
>>>
>>> if (inet_sk(sk)->inet_daddr) {
>>>     sock_rps_save_rxhash(sk, skb);
>>>     sk_mark_napi_id(sk, skb);
>>> }
>>>
>>> (To occur only for connected UDP sockets, where we are 100% sure all
>>> packets will use this same rxhash/rx queue)
>>
>> This would also be safe if there is only one NIC and said NIC was
>> programmed to always place this socket's data on the same queue.
>>
>> I don't have a good suggestion on how to detect this.
> 
> Well, this stuff relies on flows being correctly steered.
> 
> TCP stack performs much better if we avoid reorders ;)

Let's limit the discussion to UDP for now.
If you are getting packets on multiple queues for a TCP socket,
it's hard to see how you can avoid caused reordering.

Maybe it would be enough to have a socket option that the user
can set to tell the stack "I know what I'm doing, please allow busy
polling on this UDP socket, even though it's not bound".