macvlan: optimizing the receive path?

macvlan: optimizing the receive path?

[Jason Baron]

Hi,

I was just wondering why the netif_rx(skb) call in macvlan_handle_frame()
was necessary? IE:

macvlan_handle_frame()
{
       ........

        skb->dev = dev;
        skb->pkt_type = PACKET_HOST;

****>ret = netif_rx(skb);

out:
        macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
        return RX_HANDLER_CONSUMED;
}


I think the point of going through netif_rx() is to ensure that we throttle
incoming packets, but hasn't that already been accomplished in this path?
That is if the packets are arriving from the physical NIC, we've already
throttled them by this point. Otherwise, if they are coming via
macvlan_queue_xmit(), it calls either 'dev_forward_skb()', which ends
up calling netif_rx_internal(), or else in broadcast mode there is
to be throttling via macvlan_broadcast_enqueue().

So I suspect there is a code path that I am missing but the netif_rx() call in
question essentially re-queues packets coming from off the box. I've tried the
simple patch below to optimize this path, and obviously performs a lot better
in my limited testing.

Thanks,

-Jason

--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -321,8 +321,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
        skb->dev = dev;
        skb->pkt_type = PACKET_HOST;
 
-       ret = netif_rx(skb);
-
+      macvlan_count_rx(vlan, len, true, 0);
+      return RX_HANDLER_ANOTHER;
 out:
        macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
        return RX_HANDLER_CONSUMED;

Re: macvlan: optimizing the receive path?

[Stephen Hemminger]

On Thu, 02 Oct 2014 16:28:13 -0400
Jason Baron <jbaron@akamai.com> wrote:

> Hi,
> 
> I was just wondering why the netif_rx(skb) call in macvlan_handle_frame()
> was necessary? IE:


It is to prevent too deep a call stack of
  netif_receive_skb
     macvlan_receive
        netif_receive_skb ...

Re: macvlan: optimizing the receive path?

[Jason Baron]

On 10/02/2014 05:31 PM, Stephen Hemminger wrote:
> On Thu, 02 Oct 2014 16:28:13 -0400
> Jason Baron <jbaron@akamai.com> wrote:
> 
>> Hi,
>>
>> I was just wondering why the netif_rx(skb) call in macvlan_handle_frame()
>> was necessary? IE:
> 
> 
> It is to prevent too deep a call stack of
>   netif_receive_skb
>      macvlan_receive
>         netif_receive_skb ...
> 

So the concern is a stack overflow? The above nesting would really look like:

netif_receive_skb
        macvlan_receive
netif_receive_skb

b/c the macvlan_receive call would return. So its not clear to me that the
macvlan code would be adding to the stack depth here.

Thanks,

-Jason

Re: macvlan: optimizing the receive path?

[Vlad Yasevich]

On 10/02/2014 04:28 PM, Jason Baron wrote:
> Hi,
> 
> I was just wondering why the netif_rx(skb) call in macvlan_handle_frame()
> was necessary? IE:
> 
> macvlan_handle_frame()
> {
>        ........
> 
>         skb->dev = dev;
>         skb->pkt_type = PACKET_HOST;
> 
> ****>ret = netif_rx(skb);
> 
> out:
>         macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
>         return RX_HANDLER_CONSUMED;
> }
> 
> 
> I think the point of going through netif_rx() is to ensure that we throttle
> incoming packets, but hasn't that already been accomplished in this path?
> That is if the packets are arriving from the physical NIC, we've already
> throttled them by this point. Otherwise, if they are coming via
> macvlan_queue_xmit(), it calls either 'dev_forward_skb()', which ends
> up calling netif_rx_internal(), or else in broadcast mode there is
> to be throttling via macvlan_broadcast_enqueue().
> 
> So I suspect there is a code path that I am missing but the netif_rx() call in
> question essentially re-queues packets coming from off the box. I've tried the
> simple patch below to optimize this path, and obviously performs a lot better
> in my limited testing.

Hi Jason

I think the patch below is fine and in this particular case, it's OK to do this.

-vlad

> 
> Thanks,
> 
> -Jason
> 
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -321,8 +321,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>         skb->dev = dev;
>         skb->pkt_type = PACKET_HOST;
>  
> -       ret = netif_rx(skb);
> -
> +      macvlan_count_rx(vlan, len, true, 0);
> +      return RX_HANDLER_ANOTHER;
>  out:
>         macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
>         return RX_HANDLER_CONSUMED;
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: macvlan: optimizing the receive path?

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Thu, 02 Oct 2014 16:28:13 -0400

> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -321,8 +321,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>         skb->dev = dev;
>         skb->pkt_type = PACKET_HOST;
>  
> -       ret = netif_rx(skb);
> -
> +      macvlan_count_rx(vlan, len, true, 0);
> +      return RX_HANDLER_ANOTHER;
>  out:
>         macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
>         return RX_HANDLER_CONSUMED;

That last argument to macvlan_count_rx() is a bool and thus should be
specified as "false".  Yes I know other areas of this file get it
wrong too.

Also, what about GRO?  Won't we get GRO processing if we do this via
netif_rx() but not via the RX_HANDLER_ANOTHER route?  Just curious...

Re: macvlan: optimizing the receive path?

[Vlad Yasevich]

On 10/04/2014 08:42 PM, David Miller wrote:
> From: Jason Baron <jbaron@akamai.com>
> Date: Thu, 02 Oct 2014 16:28:13 -0400
> 
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -321,8 +321,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>>         skb->dev = dev;
>>         skb->pkt_type = PACKET_HOST;
>>  
>> -       ret = netif_rx(skb);
>> -
>> +      macvlan_count_rx(vlan, len, true, 0);
>> +      return RX_HANDLER_ANOTHER;
>>  out:
>>         macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
>>         return RX_HANDLER_CONSUMED;
> 
> That last argument to macvlan_count_rx() is a bool and thus should be
> specified as "false".  Yes I know other areas of this file get it
> wrong too.
> 
> Also, what about GRO?  Won't we get GRO processing if we do this via
> netif_rx() but not via the RX_HANDLER_ANOTHER route?  Just curious...

Wouldn't GRO already happen at the lower level?  For macvlan-to-macvlan,
you'd typically have large packets so no need for GRO.

-vlad

Re: macvlan: optimizing the receive path?

[Jason Baron]

On 10/06/2014 09:04 AM, Vlad Yasevich wrote:
> On 10/04/2014 08:42 PM, David Miller wrote:
>> From: Jason Baron <jbaron@akamai.com>
>> Date: Thu, 02 Oct 2014 16:28:13 -0400
>>
>>> --- a/drivers/net/macvlan.c
>>> +++ b/drivers/net/macvlan.c
>>> @@ -321,8 +321,8 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
>>>         skb->dev = dev;
>>>         skb->pkt_type = PACKET_HOST;
>>>  
>>> -       ret = netif_rx(skb);
>>> -
>>> +      macvlan_count_rx(vlan, len, true, 0);
>>> +      return RX_HANDLER_ANOTHER;
>>>  out:
>>>         macvlan_count_rx(vlan, len, ret == NET_RX_SUCCESS, 0);
>>>         return RX_HANDLER_CONSUMED;
>>
>> That last argument to macvlan_count_rx() is a bool and thus should be
>> specified as "false".  Yes I know other areas of this file get it
>> wrong too.
>>

ok. I can fix those up too while here.

>> Also, what about GRO?  Won't we get GRO processing if we do this via
>> netif_rx() but not via the RX_HANDLER_ANOTHER route?  Just curious...
> 
> Wouldn't GRO already happen at the lower level?  For macvlan-to-macvlan,
> you'd typically have large packets so no need for GRO.
> 

Yes, afaict gro is happening a layer below __netif_receive_skb_core().

Here are some results of this optimization on 3.17 using macvlan with
lxc. Test case is (average of 3 runs):

for i in {35,50,65,80,95,110,125,140,155};
do super_netperf $i netperf -H $ip -t TCP_RR;
done

trans./sec (3.17)

494016
612806
673100
696982
710494
716830
714729
713478
711056

trans./sec (3.17 + macvlan patch)

517159  +(4.684733558%)
628382  +(2.541860742%)
669688  -(0.5069080835%)
706181  +(1.319833855%)
716660  +(0.8677995555%)
719581  +(0.3838661811%)
718738  +(0.5609585358%)
718904  +(0.7605470482%)
718344  +(1.02509555%)

On the host I can see that the idle time goes to 0, so this would
appear to be an improvement. I also observed that enqueue_to_backlog()
and process_backlog() are no longer in the 'perf' profiles as
expected.

So if there are no objections, I will post as a formal patch.

Thanks,

-Jason

Re: macvlan: optimizing the receive path?

[Eric Dumazet]

On Tue, 2014-10-07 at 13:35 -0400, Jason Baron wrote:

> So if there are no objections, I will post as a formal patch.

I think the code used netif_rx() as a precaution because of
multicast/broadcasts.

Now these are taken care (if multicast filter + work queue for
broadcasts), it seems ok to have a fast path.

Re: macvlan: optimizing the receive path?

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Tue, 07 Oct 2014 13:35:42 -0400

> So if there are no objections, I will post as a formal patch.

No objections from me, thanks for all the info.