vxlan/veth performance issues on net.git + latest kernels

On Tue, 2013-12-03 at 16:55 -0800, Alexei Starovoitov wrote:
> On Tue, Dec 3, 2013 at 4:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Tue, 2013-12-03 at 16:26 -0800, Alexei Starovoitov wrote:
> >
> >>
> >> Could you use some other way to mark skb ?
> >
> > I could ;)
> >
> >> In tracing we might want to examine skb more carefully and not being
> >> able to see the device
> >> will limit the usability of this tracepoint.
> >
> > Unfortunately, using skb->dev as a pointer to device would be buggy or
> > expensive (you would need to take a reference on device in order not
> > letting it disappear, as we escape RCU protection)
> 
> well, yes, you might have an skb around when device is already freed
> when skb_dst_noref.
> but I'm not suggesting anything expensive. Tracing definitely should
> not add overhead by doing rcu_lock() or dev_hold(). Instead it can go
> through skb, skb->dev, skb->dev->xxx via probe_kernel_read(). If dev
> is gone, it's still safe.

Its certainly not safe to 'probe'.

Its not about faulting inexistent memory, that is the least of the
problem.

Any kind of information fetched from skb->dev might have been
overwritten.

You could for example fetch security sensitive data and expose it.


> 
> > Anyway, this magic is pretty easy to change, I am open to suggestions.
> 
> you're the expert :) use skb->mark field, since it's unused during
> freeing path... ?

cache line miss ;)

skb->dev is in the first cache line, where we access skb->next anyway.

I could use skb->cb[] like the following patch :





--
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

On Tue, Dec 3, 2013 at 5:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2013-12-03 at 16:55 -0800, Alexei Starovoitov wrote:
>> On Tue, Dec 3, 2013 at 4:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Tue, 2013-12-03 at 16:26 -0800, Alexei Starovoitov wrote:
>> >
>> >>
>> >> Could you use some other way to mark skb ?
>> >
>> > I could ;)
>> >
>> >> In tracing we might want to examine skb more carefully and not being
>> >> able to see the device
>> >> will limit the usability of this tracepoint.
>> >
>> > Unfortunately, using skb->dev as a pointer to device would be buggy or
>> > expensive (you would need to take a reference on device in order not
>> > letting it disappear, as we escape RCU protection)
>>
>> well, yes, you might have an skb around when device is already freed
>> when skb_dst_noref.
>> but I'm not suggesting anything expensive. Tracing definitely should
>> not add overhead by doing rcu_lock() or dev_hold(). Instead it can go
>> through skb, skb->dev, skb->dev->xxx via probe_kernel_read(). If dev
>> is gone, it's still safe.
>
> Its certainly not safe to 'probe'.
>
> Its not about faulting inexistent memory, that is the least of the
> problem.
>
> Any kind of information fetched from skb->dev might have been
> overwritten.
>
> You could for example fetch security sensitive data and expose it.

Of course.
Even without walking pointer chains with probe() you could infer all
sorts of info from tracepoints.
That's why tracing filters are for root only.

>> > Anyway, this magic is pretty easy to change, I am open to suggestions.
>>
>> you're the expert :) use skb->mark field, since it's unused during
>> freeing path... ?
>
> cache line miss ;)
>
> skb->dev is in the first cache line, where we access skb->next anyway.
>
> I could use skb->cb[] like the following patch :
>
> +struct __dev_kfree_skb_cb {
> +       unsigned int reason;
> +};
> +
> +static inline struct __dev_kfree_skb_cb *get_kfree_skb_cb(const struct sk_buff *skb)
> +{
> +       return (struct __dev_kfree_skb_cb *)skb->cb;
> +}
> +
> +enum {
> +       SKB_REASON_CONSUMED,
> +       SKB_REASON_DROPPED,
> +};
> +
>  /* Use this variant in places where it could be invoked
>   * from either hardware interrupt or other context, with hardware interrupts
>   * either disabled or enabled.
> + * Note that TX completion should use dev_consume_skb_any()
>   */
> -void dev_kfree_skb_any(struct sk_buff *skb);
> +static inline void dev_kfree_skb_any(struct sk_buff *skb)
> +{
> +       get_kfree_skb_cb(skb)->reason = SKB_REASON_DROPPED;
> +       __dev_kfree_skb_any(skb);
> +}
> +
> +static inline void dev_consume_skb_any(struct sk_buff *skb)
> +{
> +       get_kfree_skb_cb(skb)->reason = SKB_REASON_CONSUMED;
> +       __dev_kfree_skb_any(skb);
> +}

thanks. I think that is much cleaner. Ack.
--
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

On Wed, Dec 4, 2013 at 3:23 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> skb->dev is in the first cache line, where we access skb->next anyway.
> I could use skb->cb[] like the following patch :

Hi Eric, I applied on the net tree the patch you posted yesterday
"net: introduce dev_consume_skb_any()" along with the network drivers
part of this patch, unless I got it wrong, I assume both pieces are
needed?

So I re-run the vxlan/veth test that we suspect goes through packet drops on TX.

With the patches applied I have almost no samples of that event

$ ./perf report -i perf.data

Samples: 89  of event 'skb:kfree_skb', Event count (approx.): 89
+  39.33%  ksoftirqd/2  [kernel.kallsyms]  [k] net_tx_action

                    ?
+  28.09%      swapper  [kernel.kallsyms]  [k] net_tx_action

                    ?
+  28.09%         sshd  [kernel.kallsyms]  [k] net_tx_action

                    ?
+   2.25%      swapper  [kernel.kallsyms]  [k] kfree_skb

                    ?
+   1.12%  kworker/2:2  [kernel.kallsyms]  [k] net_tx_action

                    ?
+   1.12%        iperf  [kernel.kallsyms]  [k] net_tx_action

 ./perf report -i perf.data --sort dso,symbol
Samples: 89  of event 'skb:kfree_skb', Event count (approx.): 89
+  97.75%  [kernel.kallsyms]  [k] net_tx_action
+   2.25%  [kernel.kallsyms]  [k] kfree_skb
--
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