* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Willy Tarreau @ 2013-11-20 19:11 UTC (permalink / raw)
To: Arnaud Ebalard
Cc: Thomas Petazzoni, Florian Fainelli, simon.guinot, Eric Dumazet,
netdev, edumazet, Cong Wang, linux-arm-kernel
In-Reply-To: <874n780wzc.fsf@natisbad.org>
Hi Arnaud,
first, thanks for all these tests.
On Wed, Nov 20, 2013 at 12:53:43AM +0100, Arnaud Ebalard wrote:
(...)
> In the end, here are the conclusions *I* draw from this test session,
> do not hesitate to correct me:
>
> - Eric, it seems something changed in linus tree betwen the beginning
> of the thread and now, which somehow reduces the effect of the
> regression we were seen: I never got back the 256KB/s.
> - You revert patch still improves the perf a lot
> - It seems reducing MVNETA_TX_DONE_TIMER_PERIOD does not help
> - w/ your revert patch, I can confirm that mvneta driver is capable of
> doing line rate w/ proper tweak of TCP send window (256KB instead of
> 4M)
> - It seems I will I have to spend some time on the SATA issues I
> previously thought were an artefact of not cleaning my tree during a
> debug session [1], i.e. there is IMHO an issue.
Could you please try Eric's patch that was just merged into Linus' tree
if it was not yet in the kernel you tried :
98e09386c0e tcp: tsq: restore minimal amount of queueing
For me it restored the original performance (I saturate the Gbps with
about 7 concurrent streams).
Further, I wrote the small patch below for mvneta. I'm not sure it's
smp-safe but it's a PoC. In mvneta_poll() which currently is only called
upon Rx interrupt, it tries to flush all possible remaining Tx descriptors
if any. That significantly improved my transfer rate, now I easily achieve
1 Gbps using a single TCP stream on the mirabox. Not tried on the AX3 yet.
It also increased the overall connection rate by 10% on empty HTTP responses
(small packets), very likely by reducing the dead time between some segments!
You'll probably want to give it a try, so here it comes.
Cheers,
Willy
>From d1a00e593841223c7d871007b1e1fc528afe8e4d Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@1wt.eu>
Date: Wed, 20 Nov 2013 19:47:11 +0100
Subject: EXP: net: mvneta: try to flush Tx descriptor queue upon Rx
interrupts
Right now the mvneta driver doesn't handle Tx IRQ, and solely relies on a
timer to flush Tx descriptors. This causes jerky output traffic with bursts
and pauses, making it difficult to reach line rate with very few streams.
This patch tries to improve the situation which is complicated by the lack
of public datasheet from Marvell. The workaround consists in trying to flush
pending buffers during the Rx polling. The idea is that for symmetric TCP
traffic, ACKs received in response to the packets sent will trigger the Rx
interrupt and will anticipate the flushing of the descriptors.
The results are quite good, a single TCP stream is now capable of saturating
a gigabit.
This is only a workaround, it doesn't address asymmetric traffic nor datagram
based traffic.
Signed-off-by: Willy Tarreau <w@1wt.eu>
---
drivers/net/ethernet/marvell/mvneta.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 5aed8ed..59e1c86 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2013,6 +2013,26 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
}
pp->cause_rx_tx = cause_rx_tx;
+
+ /* Try to flush pending Tx buffers if any */
+ if (test_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags)) {
+ int tx_todo = 0;
+
+ mvneta_tx_done_gbe(pp,
+ (((1 << txq_number) - 1) &
+ MVNETA_CAUSE_TXQ_SENT_DESC_ALL_MASK),
+ &tx_todo);
+
+ if (tx_todo > 0) {
+ mod_timer(&pp->tx_done_timer,
+ jiffies + msecs_to_jiffies(MVNETA_TX_DONE_TIMER_PERIOD));
+ }
+ else {
+ clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
+ del_timer(&pp->tx_done_timer);
+ }
+ }
+
return rx_done;
}
--
1.7.12.1
^ permalink raw reply related
* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Arnaud Ebalard @ 2013-11-20 19:22 UTC (permalink / raw)
To: Willy Tarreau
Cc: Eric Dumazet, Thomas Petazzoni, netdev, edumazet, Cong Wang,
linux-arm-kernel
In-Reply-To: <20131120181532.GM8581@1wt.eu>
Hi,
Willy Tarreau <w@1wt.eu> writes:
> On Wed, Nov 20, 2013 at 09:40:22AM -0800, Eric Dumazet wrote:
>> On Wed, 2013-11-20 at 18:34 +0100, Willy Tarreau wrote:
>>
>> > One important point, I was looking for the other patch you pointed
>> > in this long thread and finally found it :
>> >
>> > > So
>> > > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=98e09386c0ef4dfd48af7ba60ff908f0d525cdee
>> > >
>> > > restored this minimal amount of buffering, and let the bigger amount for
>> > > 40Gb NICs ;)
>> >
>> > This one definitely restores original performance, so it's a much better
>> > bet in my opinion :-)
>>
>> I don't understand. I thought you were using this patch.
>
> No, I was on latest stable (3.10.19) which exhibits the regression but does
> not yet have your fix above. Then I tested the patch your proposed in this
> thread, then this latest one. Since the patch is not yet even in Linus'
> tree, I'm not sure Arnaud has tried it yet.
This is the one I have used for a week now, since its publication by
Eric a week ago:
http://www.spinics.net/lists/netdev/msg257396.html
Cheers,
a+
^ permalink raw reply
* [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: Paul Moore @ 2013-11-20 19:25 UTC (permalink / raw)
To: netdev
Previous commits corrected some problems with cipso_v4_translate()
when CONFIG_NETLABEL=n but some additional work is needed to tidy
things up a bit.
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
include/net/cipso_ipv4.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/cipso_ipv4.h b/include/net/cipso_ipv4.h
index a8c2ef6..2244e02 100644
--- a/include/net/cipso_ipv4.h
+++ b/include/net/cipso_ipv4.h
@@ -304,7 +304,7 @@ static inline int cipso_v4_validate(const struct sk_buff *skb,
for (opt_iter = 6; opt_iter < opt_len;) {
tag_len = opt[opt_iter + 1];
- if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))) {
+ if ((tag_len == 0) || (tag_len > (opt_len - opt_iter))) {
err_offset = opt_iter + 1;
goto out;
}
^ permalink raw reply related
* Re: [BUG,REGRESSION?] 3.11.6+,3.12: GbE iface rate drops to few KB/s
From: Arnaud Ebalard @ 2013-11-20 19:26 UTC (permalink / raw)
To: Willy Tarreau
Cc: Eric Dumazet, Thomas Petazzoni, Florian Fainelli, simon.guinot,
netdev, edumazet, Cong Wang, linux-arm-kernel
In-Reply-To: <20131120191145.GP8581@1wt.eu>
Hi,
Willy Tarreau <w@1wt.eu> writes:
> first, thanks for all these tests.
>
> On Wed, Nov 20, 2013 at 12:53:43AM +0100, Arnaud Ebalard wrote:
> (...)
>> In the end, here are the conclusions *I* draw from this test session,
>> do not hesitate to correct me:
>>
>> - Eric, it seems something changed in linus tree betwen the beginning
>> of the thread and now, which somehow reduces the effect of the
>> regression we were seen: I never got back the 256KB/s.
>> - You revert patch still improves the perf a lot
>> - It seems reducing MVNETA_TX_DONE_TIMER_PERIOD does not help
>> - w/ your revert patch, I can confirm that mvneta driver is capable of
>> doing line rate w/ proper tweak of TCP send window (256KB instead of
>> 4M)
>> - It seems I will I have to spend some time on the SATA issues I
>> previously thought were an artefact of not cleaning my tree during a
>> debug session [1], i.e. there is IMHO an issue.
>
> Could you please try Eric's patch that was just merged into Linus' tree
> if it was not yet in the kernel you tried :
>
> 98e09386c0e tcp: tsq: restore minimal amount of queueing
I have it in my quilt set.
> For me it restored the original performance (I saturate the Gbps with
> about 7 concurrent streams).
>
> Further, I wrote the small patch below for mvneta. I'm not sure it's
> smp-safe but it's a PoC. In mvneta_poll() which currently is only called
> upon Rx interrupt, it tries to flush all possible remaining Tx descriptors
> if any. That significantly improved my transfer rate, now I easily achieve
> 1 Gbps using a single TCP stream on the mirabox. Not tried on the AX3 yet.
>
> It also increased the overall connection rate by 10% on empty HTTP responses
> (small packets), very likely by reducing the dead time between some segments!
>
> You'll probably want to give it a try, so here it comes.
hehe, I was falling short of patches to test tonight ;-) I will give it
a try now.
Cheers,
a+
^ permalink raw reply
* Re: [PATCH 1/1] Workaround for Suspend/Resume issue of AX88772B under ChromeOS
From: Grant Grundler @ 2013-11-20 19:32 UTC (permalink / raw)
To: David Miller
Cc: Freddy Xin, netdev, linux-usb, LKML, davemloft,
ASIX Louis [蘇威陸], Allan Chou, Daniel_ASIX
In-Reply-To: <20131120.135413.920731121262719289.davem@davemloft.net>
On Wed, Nov 20, 2013 at 10:54 AM, David Miller <davem@davemloft.net> wrote:
> From: Grant Grundler <grundler@google.com>
> Date: Wed, 20 Nov 2013 09:42:42 -0800
>
>> While this patch raises a new issue, can you apply this patch anyway
>> since in most cases (default settings) it improves "the user
>> experience" for most users?
>
> I'm not applying a patch which knowingly is not implemented correctly.
Ok - fair enough. I'll figure out what needs to be done to
save/restore phy values.
Seems like this should be part of usbnet_resume code based on whether
the driver provides mdio_write hook (most USBNET drivers do).
thanks,
grant
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: David Miller @ 2013-11-20 19:34 UTC (permalink / raw)
To: pmoore; +Cc: netdev
In-Reply-To: <20131120192548.5616.74526.stgit@localhost>
From: Paul Moore <pmoore@redhat.com>
Date: Wed, 20 Nov 2013 14:25:48 -0500
> Previous commits corrected some problems with cipso_v4_translate()
> when CONFIG_NETLABEL=n but some additional work is needed to tidy
> things up a bit.
>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
That's really vague, please describe exactly what is wrong with the
existing conditional and how you have fixed it.
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Vlad Yasevich @ 2013-11-20 19:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20131120181949.GB25569@redhat.com>
On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>> Currently it is impossible to capture traffic when using a macvtap
>> device. The reason is that all capture handling is done either in
>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>> the lower-level device that doesn't end up matching macvtap.
>>
>> To solve the issue, use dev_hard_start_xmit() on the output path.
>> On the input path, it is toughter to solve since macvtap ends up
>> consuming the skb so there is nothing more left for
>> __netif_receive_skb_core() to do. A simple solution is to
>> pull the code that delivers things to the taps/captures into
>> its own function and allow macvtap to call it from its receive
>> routine.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> This is only an RFC. I'd like to solicit comments on this simple
>> approach.
>
> I'm kind of worried about this. What worries me is that normally
> if you have a packet socket bound to all interfaces, what it shows is
> traffic to/from the box.
>
> This might be a bug for someone, but I suspect at this point this
> is part of the ABI.
>
> But macvtap bypasses most of the host networking stack,
> So I worry that suddenly showing these packets would be confusing.
Is it really different from using bridge and tap? If someone
does 'tcpdump -i any', they will see packets sent by the guest with
bridge+tap. It makes sense to do that same thing for macvtap, no?
>
> Assuming we want to show this traffic, I think it's preferable to
> - if (!ptype->dev || ptype->dev == skb->dev) {
> + if (ptype->dev == skb->dev) {
> so you have to bind to macvtap explicitly to see the traffic.
>
>
> Of course when you start binding to specific macvtaps
> it doesn't scale well because we'll have a long list
> on ptype_all suddenly.
How likely is that really? ptype_all doesn't scale as it.
Any time you don't set the protocol field, the entry goes
into ptype_all. Macvtap doesn't change that. You can
create a long list in ptype all if you have lots of guests and
you want to snoop on each guest separately thus binding to tap
device.
-vlad
> This might mean we need to keep this list per-device ...
>
>> A more complicated solution would have been to give
>> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
>> receive operation to make the packet go through another round of
>> capturing and hit the macvtap rx_handler. I thought this would
>> hurt performance too much for no real gain.
>>
>> Thanks
>> -vlad
>>
>> drivers/net/macvtap.c | 4 +++-
>> include/linux/netdevice.h | 2 ++
>> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
>> 3 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>> index dc76670..0ed8fae 100644
>> --- a/drivers/net/macvtap.c
>> +++ b/drivers/net/macvtap.c
>> @@ -334,6 +334,7 @@ drop:
>> */
>> static int macvtap_receive(struct sk_buff *skb)
>> {
>> + netif_receive_skb_taps(skb, true);
>> skb_push(skb, ETH_HLEN);
>> return macvtap_forward(skb->dev, skb);
>> }
>> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>> }
>> if (vlan) {
>> + skb->dev = vlan->dev;
>> local_bh_disable();
>> - macvlan_start_xmit(skb, vlan->dev);
>> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
>> local_bh_enable();
>> } else {
>> kfree_skb(skb);
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 8b3de7c..84880eb 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
>> int netif_rx(struct sk_buff *skb);
>> int netif_rx_ni(struct sk_buff *skb);
>> int netif_receive_skb(struct sk_buff *skb);
>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>> + bool last_deliver);
>> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
>> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
>> struct sk_buff *napi_get_frags(struct napi_struct *napi);
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index da9c5e1..50f0ac4 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
>> }
>> }
>>
>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>> + bool last_deliver)
>> +{
>> + struct packet_type *ptype, *pt_prev = NULL;
>> + struct net_device *orig_dev;
>> +
>> + if (list_empty(&ptype_all))
>> + return NULL;
>> +
>> + orig_dev = skb->dev;
>> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
>> + if (!ptype->dev || ptype->dev == skb->dev) {
>> + if (pt_prev)
>> + deliver_skb(skb, pt_prev, orig_dev);
>> + pt_prev = ptype;
>> + }
>> + }
>> +
>> + if (last_deliver && pt_prev)
>> + deliver_skb(skb, pt_prev, orig_dev);
>> +
>> + return pt_prev;
>> +}
>> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
>> +
>> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
>> {
>> struct packet_type *ptype, *pt_prev;
>> @@ -3535,13 +3560,7 @@ another_round:
>> if (pfmemalloc)
>> goto skip_taps;
>>
>> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
>> - if (!ptype->dev || ptype->dev == skb->dev) {
>> - if (pt_prev)
>> - ret = deliver_skb(skb, pt_prev, orig_dev);
>> - pt_prev = ptype;
>> - }
>> - }
>> + pt_prev = netif_receive_skb_taps(skb, false);
>>
>> skip_taps:
>> #ifdef CONFIG_NET_CLS_ACT
>> --
>> 1.8.4.2
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: Paul Moore @ 2013-11-20 19:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20131120.143407.4095832971724166.davem@davemloft.net>
On Wednesday, November 20, 2013 02:34:07 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Wed, 20 Nov 2013 14:25:48 -0500
>
> > Previous commits corrected some problems with cipso_v4_translate()
> > when CONFIG_NETLABEL=n but some additional work is needed to tidy
> > things up a bit.
> >
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
>
> That's really vague, please describe exactly what is wrong with the
> existing conditional and how you have fixed it.
I kinda figured the one line patch and "some additional work is needed to tidy
things up a bit" summed it up nicely, but I guess not so here ya go ...
First, for reference, here is the diff one more time (some whitespace damage
in the paste below for readability):
> diff --git a/include/net/cipso_ipv4.h b/include/net/cipso_ipv4.h
> index a8c2ef6..2244e02 100644
> --- a/include/net/cipso_ipv4.h
> +++ b/include/net/cipso_ipv4.h
> @@ -304,7 +304,7 @@ static inline int cipso_v4_validate(...)
> for (opt_iter = 6; opt_iter < opt_len;) {
> tag_len = opt[opt_iter + 1];
>
> - if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))) {
> + if ((tag_len == 0) || (tag_len > (opt_len - opt_iter))) {
> err_offset = opt_iter + 1;
> goto out;
> }
Looking at the original conditional:
if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))
... and the replacement:
if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))
... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been
replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len' for
'opt[opt_iter + 1]'. This is acceptable because the the first statement in
the for loop is:
tag_len = opt[opt_iter + 1]
... which matches the substitution in the conditional. I'm not sure how much
more explicit I can be about this change, it is really pretty minor.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: David Miller @ 2013-11-20 19:52 UTC (permalink / raw)
To: mst; +Cc: vyasevic, netdev
In-Reply-To: <20131120181949.GB25569@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Wed, 20 Nov 2013 20:19:49 +0200
> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>> Currently it is impossible to capture traffic when using a macvtap
>> device. The reason is that all capture handling is done either in
>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>> the lower-level device that doesn't end up matching macvtap.
>>
>> To solve the issue, use dev_hard_start_xmit() on the output path.
>> On the input path, it is toughter to solve since macvtap ends up
>> consuming the skb so there is nothing more left for
>> __netif_receive_skb_core() to do. A simple solution is to
>> pull the code that delivers things to the taps/captures into
>> its own function and allow macvtap to call it from its receive
>> routine.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> This is only an RFC. I'd like to solicit comments on this simple
>> approach.
>
> I'm kind of worried about this. What worries me is that normally
> if you have a packet socket bound to all interfaces, what it shows is
> traffic to/from the box.
>
> This might be a bug for someone, but I suspect at this point this
> is part of the ABI.
Tunnel decapsulations on input are shown for other types of devices,
such as IP tunnels. It is because they feed packets back into the
stack via netif_receive_skb() upon decapsulation.
Then we have all of this sideways code for VLANs and "rx_handler"s
in order to perform decapsulation via direct iteration instead of
recursion inside of __netif_receive_skb_core().
I suspect that Vlad's suggested rx_handler alternative approach is
going to be much better.
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: David Miller @ 2013-11-20 19:55 UTC (permalink / raw)
To: pmoore; +Cc: netdev
In-Reply-To: <4007061.3MtKnenLV1@sifl>
From: Paul Moore <pmoore@redhat.com>
Date: Wed, 20 Nov 2013 14:45:19 -0500
> Looking at the original conditional:
>
> if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))
>
> ... and the replacement:
>
> if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))
>
> ... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been
> replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len' for
> 'opt[opt_iter + 1]'. This is acceptable because the the first statement in
> the for loop is:
>
> tag_len = opt[opt_iter + 1]
>
> ... which matches the substitution in the conditional. I'm not sure how much
> more explicit I can be about this change, it is really pretty minor.
Then, two things:
1) This is a cleanup, and therefore not suitable for submission right now
because the net-next tree is closed.
2) A more suitable commit log message would have been "Don't needlessly
recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
Then anyone who reads this commit message can say "yes, obviously this
is a correct change and matches what the patch is doing"
Thanks.
^ permalink raw reply
* Re: [RFC net-next] 6lowpan: Moving generic parts into net/core/6lowpan.c
From: David Miller @ 2013-11-20 19:58 UTC (permalink / raw)
To: jukka.rissanen; +Cc: netdev, alex.aring
In-Reply-To: <1384950849-1982-1-git-send-email-jukka.rissanen@linux.intel.com>
From: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Date: Wed, 20 Nov 2013 14:34:09 +0200
> Because the IEEE 802154 and Bluetooth 6LoWPAN share the IPv6
> header compression and uncompression code, the common code
> is moved to net/core/6lowpan.c file.
>
> Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
There is no reason to move this file out from net/ieee802154/
Just like ipv6 does, you can have a net/ieee802154/6lowpan_core.c
file and appropriate Kconfig/Makefile rules to split this functionality
out appropriately.
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: Paul Moore @ 2013-11-20 19:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20131120.145511.1303114952026497883.davem@davemloft.net>
On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Wed, 20 Nov 2013 14:45:19 -0500
>
> > Looking at the original conditional:
> > if ((tag_len == 0) || (opt[opt_iter + 1] > (opt_len - opt_iter))
> >
> > ... and the replacement:
> > if ((tag_len == 0) || (tag_len > (opt_len - opt_iter)))
> >
> > ... we notice that "(opt[opt_iter + 1] > (opt_len - opt_iter))" has been
> > replaced with "(tag_len > (opt_len - opt_iter))", substituting 'tag_len'
> > for 'opt[opt_iter + 1]'. This is acceptable because the the first
> > statement in>
> > the for loop is:
> > tag_len = opt[opt_iter + 1]
> >
> > ... which matches the substitution in the conditional. I'm not sure how
> > much more explicit I can be about this change, it is really pretty minor.
>
> Then, two things:
>
> 1) This is a cleanup, and therefore not suitable for submission right now
> because the net-next tree is closed.
I wasn't expecting it to go into the current merge window, I was just sending
it out now so you'd have it for when things started moving again. Think of it
as spreading the pain out a bit.
> 2) A more suitable commit log message would have been "Don't needlessly
> recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
>
> Then anyone who reads this commit message can say "yes, obviously this
> is a correct change and matches what the patch is doing"
Fair enough. Do you want a resubmit with your wording once the merge window
closes?
> Thanks.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Michael S. Tsirkin @ 2013-11-20 20:06 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev
In-Reply-To: <528D0F48.60304@redhat.com>
On Wed, Nov 20, 2013 at 02:36:40PM -0500, Vlad Yasevich wrote:
> On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
> >> Currently it is impossible to capture traffic when using a macvtap
> >> device. The reason is that all capture handling is done either in
> >> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
> >> currenlty doesn't use dev_hard_start_xmit(), and at the time the
> >> packet traverses __netif_receive_skb_core, the skb->dev is set to
> >> the lower-level device that doesn't end up matching macvtap.
> >>
> >> To solve the issue, use dev_hard_start_xmit() on the output path.
> >> On the input path, it is toughter to solve since macvtap ends up
> >> consuming the skb so there is nothing more left for
> >> __netif_receive_skb_core() to do.
Actually I thought I understand what you are saying here, but now I
don't. bridge installs rx handler exactly in the same way.
packet handlers seem to be called before the rx handlers so
everything should just work.
Is this about the bridge mode again?
> A simple solution is to
> >> pull the code that delivers things to the taps/captures into
> >> its own function and allow macvtap to call it from its receive
> >> routine.
> >>
> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> >> ---
> >> This is only an RFC. I'd like to solicit comments on this simple
> >> approach.
> >
> > I'm kind of worried about this. What worries me is that normally
> > if you have a packet socket bound to all interfaces, what it shows is
> > traffic to/from the box.
> >
> > This might be a bug for someone, but I suspect at this point this
> > is part of the ABI.
> >
> > But macvtap bypasses most of the host networking stack,
> > So I worry that suddenly showing these packets would be confusing.
>
> Is it really different from using bridge and tap? If someone
> does 'tcpdump -i any', they will see packets sent by the guest with
> bridge+tap.
> It makes sense to do that same thing for macvtap, no?
I was going by your comments not the code.
Assuming we never showed macvtap traffic this might
be part of ABI.
> >
> > Assuming we want to show this traffic, I think it's preferable to
> > - if (!ptype->dev || ptype->dev == skb->dev) {
> > + if (ptype->dev == skb->dev) {
> > so you have to bind to macvtap explicitly to see the traffic.
> >
> >
> > Of course when you start binding to specific macvtaps
> > it doesn't scale well because we'll have a long list
> > on ptype_all suddenly.
>
> How likely is that really? ptype_all doesn't scale as it.
> Any time you don't set the protocol field, the entry goes
> into ptype_all. Macvtap doesn't change that. You can
> create a long list in ptype all if you have lots of guests and
> you want to snoop on each guest separately thus binding to tap
> device.
>
> -vlad
>
> > This might mean we need to keep this list per-device ...
> >
> >> A more complicated solution would have been to give
> >> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
> >> receive operation to make the packet go through another round of
> >> capturing and hit the macvtap rx_handler. I thought this would
> >> hurt performance too much for no real gain.
> >>
> >> Thanks
> >> -vlad
> >>
> >> drivers/net/macvtap.c | 4 +++-
> >> include/linux/netdevice.h | 2 ++
> >> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
> >> 3 files changed, 31 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> >> index dc76670..0ed8fae 100644
> >> --- a/drivers/net/macvtap.c
> >> +++ b/drivers/net/macvtap.c
> >> @@ -334,6 +334,7 @@ drop:
> >> */
> >> static int macvtap_receive(struct sk_buff *skb)
> >> {
> >> + netif_receive_skb_taps(skb, true);
> >> skb_push(skb, ETH_HLEN);
> >> return macvtap_forward(skb->dev, skb);
> >> }
> >> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
> >> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
> >> }
> >> if (vlan) {
> >> + skb->dev = vlan->dev;
> >> local_bh_disable();
> >> - macvlan_start_xmit(skb, vlan->dev);
> >> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
> >> local_bh_enable();
> >> } else {
> >> kfree_skb(skb);
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index 8b3de7c..84880eb 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
> >> int netif_rx(struct sk_buff *skb);
> >> int netif_rx_ni(struct sk_buff *skb);
> >> int netif_receive_skb(struct sk_buff *skb);
> >> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
> >> + bool last_deliver);
> >> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
> >> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
> >> struct sk_buff *napi_get_frags(struct napi_struct *napi);
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index da9c5e1..50f0ac4 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
> >> }
> >> }
> >>
> >> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
> >> + bool last_deliver)
> >> +{
> >> + struct packet_type *ptype, *pt_prev = NULL;
> >> + struct net_device *orig_dev;
> >> +
> >> + if (list_empty(&ptype_all))
> >> + return NULL;
> >> +
> >> + orig_dev = skb->dev;
> >> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
> >> + if (!ptype->dev || ptype->dev == skb->dev) {
> >> + if (pt_prev)
> >> + deliver_skb(skb, pt_prev, orig_dev);
> >> + pt_prev = ptype;
> >> + }
> >> + }
> >> +
> >> + if (last_deliver && pt_prev)
> >> + deliver_skb(skb, pt_prev, orig_dev);
> >> +
> >> + return pt_prev;
> >> +}
> >> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
> >> +
> >> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
> >> {
> >> struct packet_type *ptype, *pt_prev;
> >> @@ -3535,13 +3560,7 @@ another_round:
> >> if (pfmemalloc)
> >> goto skip_taps;
> >>
> >> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
> >> - if (!ptype->dev || ptype->dev == skb->dev) {
> >> - if (pt_prev)
> >> - ret = deliver_skb(skb, pt_prev, orig_dev);
> >> - pt_prev = ptype;
> >> - }
> >> - }
> >> + pt_prev = netif_receive_skb_taps(skb, false);
> >>
> >> skip_taps:
> >> #ifdef CONFIG_NET_CLS_ACT
> >> --
> >> 1.8.4.2
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: David Miller @ 2013-11-20 20:06 UTC (permalink / raw)
To: pmoore; +Cc: netdev
In-Reply-To: <2049996.Ospk96ghpo@sifl>
From: Paul Moore <pmoore@redhat.com>
Date: Wed, 20 Nov 2013 14:59:33 -0500
> On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote:
>> 1) This is a cleanup, and therefore not suitable for submission right now
>> because the net-next tree is closed.
>
> I wasn't expecting it to go into the current merge window, I was just sending
> it out now so you'd have it for when things started moving again. Think of it
> as spreading the pain out a bit.
When the merge window opens, I make an announcement here on the list
politely asking pople to defer net-next patch submissions until after
the merge window closes and I explicitly announce that net-next is
open again.
>> 2) A more suitable commit log message would have been "Don't needlessly
>> recompute 'opt[opt_iter + 1]' as we already have it stored in tag_len".
>>
>> Then anyone who reads this commit message can say "yes, obviously this
>> is a correct change and matches what the patch is doing"
>
> Fair enough. Do you want a resubmit with your wording once the merge window
> closes?
... and I announce that net-next is open once more, yes. Thank you.
^ permalink raw reply
* Re: [PATCH net] packet: fix use after free race in send path when dev is released
From: David Miller @ 2013-11-20 20:07 UTC (permalink / raw)
To: dborkman; +Cc: netdev, noureddine, greearb
In-Reply-To: <528C7319.5050309@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 20 Nov 2013 09:30:17 +0100
> On 11/20/2013 02:34 AM, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Wed, 20 Nov 2013 00:08:23 +0100
>>
>>> To avoid reverting 827d9780 entirely, we could make use of po->running
>>> member that gets reset when we're calling __unregister_prot_hook() in
>>> packet_notifier() when we receive NETDEV_DOWN or NETDEV_UNREGISTER
>>> notification. Plus, we still need to hold ref to the netdev, so
>>> that we can assure it won't be released while we're in send path.
>>
>> The avoidance of the atomic ref counting of the network device is the
>> main performance gain we get from that commit.
>>
>> Now we'll be doing the refcount _and_ taking a spinlock, it'll be
>> worse than beforehand.
>>
>> And this is doubly silly because we already have a reference
>> when we install the device into po->prot_hook.dev
>>
>> I bet you can fix this by just deferring the NETDEV_UNREGISTER
>> AF_PACKET notifier work to RCU.
>
> Yep, will try if this approach works, in other words doing the earlier
> exit via !po->running, plus deferring the dev_put() et al to RCU.
Thank you. You might have to wrap the sendmsg path in an rcu lock
sequence.
^ permalink raw reply
* Re: [PATCH] cipso: simplify cipso_v4_translate() when !CONFIG_NETLABEL
From: Paul Moore @ 2013-11-20 20:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20131120.150620.1185820553046155927.davem@davemloft.net>
On Wednesday, November 20, 2013 03:06:20 PM David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Wed, 20 Nov 2013 14:59:33 -0500
>
> > On Wednesday, November 20, 2013 02:55:11 PM David Miller wrote:
> >> 1) This is a cleanup, and therefore not suitable for submission right now
> >> because the net-next tree is closed.
> >
> > I wasn't expecting it to go into the current merge window, I was just
> > sending it out now so you'd have it for when things started moving again.
> > Think of it as spreading the pain out a bit.
>
> When the merge window opens, I make an announcement here on the list
> politely asking pople to defer net-next patch submissions until after
> the merge window closes and I explicitly announce that net-next is
> open again.
My apologies, I missed that. Subscribed to too many mailing lists and too
reliant on my filters to catch the important bits.
> >> 2) A more suitable commit log message would have been "Don't needlessly
> >> recompute 'opt[opt_iter + 1]' as we already have it stored in
> >> tag_len".
> >>
> >> Then anyone who reads this commit message can say "yes, obviously this
> >> is a correct change and matches what the patch is doing"
> >
> > Fair enough. Do you want a resubmit with your wording once the merge
> > window closes?
>
> ... and I announce that net-next is open once more, yes. Thank you.
Will do.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Vlad Yasevich @ 2013-11-20 20:10 UTC (permalink / raw)
To: David Miller, mst; +Cc: netdev
In-Reply-To: <20131120.145233.1158930529349207237.davem@davemloft.net>
On 11/20/2013 02:52 PM, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Wed, 20 Nov 2013 20:19:49 +0200
>
>> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>>> Currently it is impossible to capture traffic when using a macvtap
>>> device. The reason is that all capture handling is done either in
>>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>>> the lower-level device that doesn't end up matching macvtap.
>>>
>>> To solve the issue, use dev_hard_start_xmit() on the output path.
>>> On the input path, it is toughter to solve since macvtap ends up
>>> consuming the skb so there is nothing more left for
>>> __netif_receive_skb_core() to do. A simple solution is to
>>> pull the code that delivers things to the taps/captures into
>>> its own function and allow macvtap to call it from its receive
>>> routine.
>>>
>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>> ---
>>> This is only an RFC. I'd like to solicit comments on this simple
>>> approach.
>>
>> I'm kind of worried about this. What worries me is that normally
>> if you have a packet socket bound to all interfaces, what it shows is
>> traffic to/from the box.
>>
>> This might be a bug for someone, but I suspect at this point this
>> is part of the ABI.
>
> Tunnel decapsulations on input are shown for other types of devices,
> such as IP tunnels. It is because they feed packets back into the
> stack via netif_receive_skb() upon decapsulation.
>
> Then we have all of this sideways code for VLANs and "rx_handler"s
> in order to perform decapsulation via direct iteration instead of
> recursion inside of __netif_receive_skb_core().
>
> I suspect that Vlad's suggested rx_handler alternative approach is
> going to be much better.
>
Hi David
I don't know if "better" is what I'd say here. With the current code,
if no-one is capturing, the cost is that of "if list_empty". If
I switch to rx_handler approach, the cost goes up on every packet even
if no-one is capture. The call stack ends up beeing really silly:
_netif_receive_skb_core()
macvlan_handle_frame()
macvtap_receive()
return RX_HANDLER_ANOTHER;
macvtap_handle_frame()
consume.
Yes, this approach seems to fit in better with the architecture of the
stack, but boy, it looks inefficient.
Where as we were able to steal frames before in macvtap_receive, we now
have to go around one more time.
I am going to prototype this and see what the numbers look like, but it
seems such an overkill.
Thanks
-vlad
^ permalink raw reply
* Re: [PATCH net] packet: fix use after free race in send path when dev is released
From: Eric Dumazet @ 2013-11-20 20:15 UTC (permalink / raw)
To: David Miller; +Cc: dborkman, netdev, noureddine, greearb
In-Reply-To: <20131120.150711.1110320732127884184.davem@davemloft.net>
On Wed, 2013-11-20 at 15:07 -0500, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> > Yep, will try if this approach works, in other words doing the earlier
> > exit via !po->running, plus deferring the dev_put() et al to RCU.
>
> Thank you. You might have to wrap the sendmsg path in an rcu lock
> sequence.
Note that dev_put() / dev_hold() are blazingly fast ;)
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: Vlad Yasevich @ 2013-11-20 20:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev
In-Reply-To: <20131120200658.GA11070@redhat.com>
On 11/20/2013 03:06 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 20, 2013 at 02:36:40PM -0500, Vlad Yasevich wrote:
>> On 11/20/2013 01:19 PM, Michael S. Tsirkin wrote:
>>> On Wed, Nov 20, 2013 at 01:04:09PM -0500, Vlad Yasevich wrote:
>>>> Currently it is impossible to capture traffic when using a macvtap
>>>> device. The reason is that all capture handling is done either in
>>>> dev_hard_start_xmit() or in __netif_receive_skb_core(). Macvtap
>>>> currenlty doesn't use dev_hard_start_xmit(), and at the time the
>>>> packet traverses __netif_receive_skb_core, the skb->dev is set to
>>>> the lower-level device that doesn't end up matching macvtap.
>>>>
>>>> To solve the issue, use dev_hard_start_xmit() on the output path.
>>>> On the input path, it is toughter to solve since macvtap ends up
>>>> consuming the skb so there is nothing more left for
>>>> __netif_receive_skb_core() to do.
>
> Actually I thought I understand what you are saying here, but now I
> don't. bridge installs rx handler exactly in the same way.
> packet handlers seem to be called before the rx handlers so
> everything should just work.
>
> Is this about the bridge mode again?
No. It has to do with bridge submitting the frame back to the
network stack to forward it to the ports, but macvtap ends up
stealing it.
Also, bridge, if running in promisc mode will hand the frame up
(as if it received it). See the IFF_PROMISC code in
br_handle_frame_finish().
So, if someone is capturing on the bridge itself, this lets it
see the packets even though they are not destined for the bridge
device.
>
>> A simple solution is to
>>>> pull the code that delivers things to the taps/captures into
>>>> its own function and allow macvtap to call it from its receive
>>>> routine.
>>>>
>>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>>> ---
>>>> This is only an RFC. I'd like to solicit comments on this simple
>>>> approach.
>>>
>>> I'm kind of worried about this. What worries me is that normally
>>> if you have a packet socket bound to all interfaces, what it shows is
>>> traffic to/from the box.
>>>
>>> This might be a bug for someone, but I suspect at this point this
>>> is part of the ABI.
>>>
>>> But macvtap bypasses most of the host networking stack,
>>> So I worry that suddenly showing these packets would be confusing.
>>
>> Is it really different from using bridge and tap? If someone
>> does 'tcpdump -i any', they will see packets sent by the guest with
>> bridge+tap.
>> It makes sense to do that same thing for macvtap, no?
>
> I was going by your comments not the code.
> Assuming we never showed macvtap traffic this might
> be part of ABI.
BTW, if we end up doing it with a new rx_handler, it will end up
showing exactly the same traffic is this patch does because the
common path in __netif_receive_skb_core() will run and deliver
to all registered eligible entries in ptype_all.
-vlad
>
>>>
>>> Assuming we want to show this traffic, I think it's preferable to
>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>> + if (ptype->dev == skb->dev) {
>>> so you have to bind to macvtap explicitly to see the traffic.
>>>
>>>
>>> Of course when you start binding to specific macvtaps
>>> it doesn't scale well because we'll have a long list
>>> on ptype_all suddenly.
>>
>> How likely is that really? ptype_all doesn't scale as it.
>> Any time you don't set the protocol field, the entry goes
>> into ptype_all. Macvtap doesn't change that. You can
>> create a long list in ptype all if you have lots of guests and
>> you want to snoop on each guest separately thus binding to tap
>> device.
>>
>> -vlad
>>
>>> This might mean we need to keep this list per-device ...
>>>
>>>> A more complicated solution would have been to give
>>>> macvtap its own rx_handler and return RX_HANDLER_ANOTHER during
>>>> receive operation to make the packet go through another round of
>>>> capturing and hit the macvtap rx_handler. I thought this would
>>>> hurt performance too much for no real gain.
>>>>
>>>> Thanks
>>>> -vlad
>>>>
>>>> drivers/net/macvtap.c | 4 +++-
>>>> include/linux/netdevice.h | 2 ++
>>>> net/core/dev.c | 33 ++++++++++++++++++++++++++-------
>>>> 3 files changed, 31 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
>>>> index dc76670..0ed8fae 100644
>>>> --- a/drivers/net/macvtap.c
>>>> +++ b/drivers/net/macvtap.c
>>>> @@ -334,6 +334,7 @@ drop:
>>>> */
>>>> static int macvtap_receive(struct sk_buff *skb)
>>>> {
>>>> + netif_receive_skb_taps(skb, true);
>>>> skb_push(skb, ETH_HLEN);
>>>> return macvtap_forward(skb->dev, skb);
>>>> }
>>>> @@ -727,8 +728,9 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
>>>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>>>> }
>>>> if (vlan) {
>>>> + skb->dev = vlan->dev;
>>>> local_bh_disable();
>>>> - macvlan_start_xmit(skb, vlan->dev);
>>>> + dev_hard_start_xmit(skb, vlan->dev, NULL, NULL);
>>>> local_bh_enable();
>>>> } else {
>>>> kfree_skb(skb);
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 8b3de7c..84880eb 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -2383,6 +2383,8 @@ void dev_kfree_skb_any(struct sk_buff *skb);
>>>> int netif_rx(struct sk_buff *skb);
>>>> int netif_rx_ni(struct sk_buff *skb);
>>>> int netif_receive_skb(struct sk_buff *skb);
>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>> + bool last_deliver);
>>>> gro_result_t napi_gro_receive(struct napi_struct *napi, struct sk_buff *skb);
>>>> void napi_gro_flush(struct napi_struct *napi, bool flush_old);
>>>> struct sk_buff *napi_get_frags(struct napi_struct *napi);
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index da9c5e1..50f0ac4 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -3484,6 +3484,31 @@ static bool skb_pfmemalloc_protocol(struct sk_buff *skb)
>>>> }
>>>> }
>>>>
>>>> +struct packet_type *netif_receive_skb_taps(struct sk_buff *skb,
>>>> + bool last_deliver)
>>>> +{
>>>> + struct packet_type *ptype, *pt_prev = NULL;
>>>> + struct net_device *orig_dev;
>>>> +
>>>> + if (list_empty(&ptype_all))
>>>> + return NULL;
>>>> +
>>>> + orig_dev = skb->dev;
>>>> + list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>> + if (!ptype->dev || ptype->dev == skb->dev) {
>>>> + if (pt_prev)
>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>> + pt_prev = ptype;
>>>> + }
>>>> + }
>>>> +
>>>> + if (last_deliver && pt_prev)
>>>> + deliver_skb(skb, pt_prev, orig_dev);
>>>> +
>>>> + return pt_prev;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(netif_receive_skb_taps);
>>>> +
>>>> static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc)
>>>> {
>>>> struct packet_type *ptype, *pt_prev;
>>>> @@ -3535,13 +3560,7 @@ another_round:
>>>> if (pfmemalloc)
>>>> goto skip_taps;
>>>>
>>>> - list_for_each_entry_rcu(ptype, &ptype_all, list) {
>>>> - if (!ptype->dev || ptype->dev == skb->dev) {
>>>> - if (pt_prev)
>>>> - ret = deliver_skb(skb, pt_prev, orig_dev);
>>>> - pt_prev = ptype;
>>>> - }
>>>> - }
>>>> + pt_prev = netif_receive_skb_taps(skb, false);
>>>>
>>>> skip_taps:
>>>> #ifdef CONFIG_NET_CLS_ACT
>>>> --
>>>> 1.8.4.2
^ permalink raw reply
* Re: [RFC net-next PATCH] macvtap: Add packet capture support
From: David Miller @ 2013-11-20 20:20 UTC (permalink / raw)
To: vyasevic; +Cc: mst, netdev
In-Reply-To: <528D173F.2060102@redhat.com>
From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 20 Nov 2013 15:10:39 -0500
> I don't know if "better" is what I'd say here. With the current code,
> if no-one is capturing, the cost is that of "if list_empty". If
> I switch to rx_handler approach, the cost goes up on every packet even
> if no-one is capture. The call stack ends up beeing really silly:
> _netif_receive_skb_core()
> macvlan_handle_frame()
> macvtap_receive()
> return RX_HANDLER_ANOTHER;
> macvtap_handle_frame()
> consume.
>
> Yes, this approach seems to fit in better with the architecture of the
> stack, but boy, it looks inefficient.
It is cheaper than:
> macvlan_handle_frame()
> macvtap_receive()
> netif_receive_skb()
> ....
> reprocess software interrupt
etc. etc.
With the rx_handler approach we're just iterating to a function call
which consumes the SKB, that whole code path is pretty much guarenteed
to be in the CPUs I-cache the second time.
I look forward to seeing your perf data :)
^ permalink raw reply
* Re: [PATCH net] packet: fix use after free race in send path when dev is released
From: David Miller @ 2013-11-20 20:21 UTC (permalink / raw)
To: eric.dumazet; +Cc: dborkman, netdev, noureddine, greearb
In-Reply-To: <1384978559.10637.24.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Nov 2013 12:15:59 -0800
> On Wed, 2013-11-20 at 15:07 -0500, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>
>> > Yep, will try if this approach works, in other words doing the earlier
>> > exit via !po->running, plus deferring the dev_put() et al to RCU.
>>
>> Thank you. You might have to wrap the sendmsg path in an rcu lock
>> sequence.
>
> Note that dev_put() / dev_hold() are blazingly fast ;)
Oh yes, per-cpu counters, almost forgot :-)))
^ permalink raw reply
* [PATCH RFC v1 0/7] net: phy: Ethernet PHY powerdown optimization
From: Sebastian Hesselbarth @ 2013-11-20 20:21 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Sebastian Hesselbarth, David S. Miller, netdev, linux-arm-kernel,
linux-kernel
Ethernet PHYs consume a significant amount of power when link is detected.
Especially, for embedded systems it can be easily 20-40% of total system
power. Now, currently most likely all ethernet drivers leave PHYs powered
on, even if the device is taken down. Also, some stupid boot loaders power
on all PHYs available.
This RFC deals with saving power consumed by ethernet PHYs, that have no
corresponding ethernet device driver or are attached to ethernet devices
which are taken down by user request, i.e. ifconfig ethN down. Ports with
no link, i.e. cable removed, are already quite good at power saving due to
PHY internal link detection.
I did some power measurements for 4 different boards with 3 different
drivers (all ARM) and reduction in power dissipation is usually ~500mW
per port. Guruplug savings are a bit high, but while re-testing v3.12
it just crashed because of heat issues - a known problem with this board
so don't take the absolute values too serious.
Now, the reason why I send this as a RFC and not a real patch set is,
that I started with mv643xx_eth which is an "evolutionary piece of code"
and phy support isn't that well done. When touching mvneta/cpsw, I saw
that phy_suspend/resume should possibly better be hidden in phy_start
and phy_stop, respectively. Also, suspending unused PHYs has the potential
to break stuff and I only had some ARM boards available to test.
Anyway, it would be great to get some general feed-back for the current
approach and possible improvements.
The individual patches can be summarized as follows:
Patch 1 adds genphy suspend/resume callbacks to Marvell PHYs.
Patch 2 introduces phy_resume and phy_suspend helpers to ease calling
the corresponding PHY driver callbacks, if available.
Patch 3 calls above helpers to automatically resume PHYs on
phy_attach and suspend them on phy_detach.
Patch 4 adds a late_initcall to suspend unused PHYs that have not been
taken by any driver. This is what we already have for unused clocks in
the common clock framework.
Patch 5, 6, and 7 add phy_resume/suspend to the three individual drivers
on their open/stop callbacks. This suspends the PHY as soon as the ethernet
device is taken down by ifconfig ethN down.
Below are the results of recent linux/master against this RFC. All links
are 1Gbps except Beaglebone Black wich is 100Mbps only.
A branch with the RFC applied can also be found at:
https://github.com/shesselba/linux-dove.git topic/ethphy-power-rfc-v1
Sebastian
(a) SolidRun CuBox, Marvell Dove, Marvell 88E1310, mv643xx_eth
v3.12 +RFC diff/abs diff/rel
1 port, link, up 2115 mW 2116 mW + 1 mW + 0.0%
1 port, link, down 2104 mW 1572 mW -532 mW -25.3%
1 port, no link, up 1600 mW 1614 mW + 14 mW + 0.9%
1 port, no link, down 1604 mW 1577 mW - 27 mW - 1.7%
(b) Beaglebone Black, TI AM3359, SMSC LAN8710/8720, cpsw (100Mbps)
v3.12 +RFC diff/abs diff/rel
1 port, link, up 1247 mW 1248 mW + 1 mW + 0.1%
1 port, link, down 1247 mW 808 mW -439 mW -35.2%
1 port, no link, up 817 mW 817 mW + 0 mW + 0.0%
1 port, no link, down 808 mW 809 mW + 1 mW + 0.1%
(c) Globalscale Guruplug, Marvell Kirkwood, Marvell 88E1121, mv643xx_eth
v3.12 +RFC diff/abs diff/rel
2 ports, link, up 6611 mW 6587 mW - 24 mW - 0.4%
2 ports, link, down 6625 mW 3881 mW -2744 mW -41.4%
2 ports, no link, up 4072 mW 4002 mW - 70 mW - 1.7%
2 ports, no link, down 4085 mW 3875 mW - 210 mW - 5.1%
1 port, link, up
1 port, link, down 6622 mW 5193 mW -1429 mW -21.6%
1 port, link, up
1 port, no link, down 5276 mW 5193 mW - 83 mW - 1.6%
1 port, no link, up
1 port, no link, down 4029 mW 3941 mW - 88 mW - 2.2%
(d) Globalscale Mirabox, Marvell Armada 370, Marvell 88E1510, mvneta
v3.12 +RFC diff/abs diff/rel
2 ports, link, up 5218 mW 5177 mW - 41 mW - 0.8%
2 ports, link, down 5216 mW 4225 mW -991 mW -19.0%
2 ports, no link, up 4352 mW 4320 mW - 32 mW - 0.7%
2 ports, no link, down 4352 mW 4228 mW -124 mW - 2.8%
1 port, link, up
1 port, link, down 5218 mW 4718 mW -500 mW - 9.6%
1 port, link, up
1 port, no link, down 4842 mW 4722 mW -120 mW - 2.5%
1 port, no link, up
1 port, no link, down 4356 mW 4290 mW - 66 mW - 1.5%
Sebastian Hesselbarth (7):
net: phy: marvell: provide genphy suspend/resume
net: phy: provide phy_resume/phy_suspend helpers
net: phy: resume/suspend PHYs on attach/detach
net: phy: suspend unused PHYs on mdio_bus in late_initcall
net: mv643xx_eth: resume/suspend PHY on port start/stop
net: mvneta: resume/suspend PHY on port start/stop
net: cpsw: resume/suspend PHY on port start/stop
drivers/net/ethernet/marvell/mv643xx_eth.c | 7 +++++++
drivers/net/ethernet/marvell/mvneta.c | 2 ++
drivers/net/ethernet/ti/cpsw.c | 2 ++
drivers/net/phy/marvell.c | 22 ++++++++++++++++++++++
drivers/net/phy/mdio_bus.c | 27 +++++++++++++++++++++++++++
drivers/net/phy/phy_device.c | 22 ++++++++++++++++++++++
include/linux/phy.h | 2 ++
7 files changed, 84 insertions(+), 0 deletions(-)
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
--
1.7.2.5
^ permalink raw reply
* [PATCH RFC v1 1/7] net: phy: marvell: provide genphy suspend/resume
From: Sebastian Hesselbarth @ 2013-11-20 20:21 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: netdev, linux-kernel, David S. Miller, linux-arm-kernel,
Sebastian Hesselbarth
In-Reply-To: <1384978913-8052-1-git-send-email-sebastian.hesselbarth@gmail.com>
Marvell PHYs support generic PHY suspend/resume, so provide those
callbacks to all marvell specific drivers.
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/phy/marvell.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 2e3c778e..bd37e45 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -894,6 +894,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -907,6 +909,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -920,6 +924,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &marvell_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -933,6 +939,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = {.owner = THIS_MODULE,},
},
{
@@ -946,6 +954,8 @@ static struct phy_driver marvell_drivers[] = {
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
.did_interrupt = &m88e1121_did_interrupt,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -961,6 +971,8 @@ static struct phy_driver marvell_drivers[] = {
.did_interrupt = &m88e1121_did_interrupt,
.get_wol = &m88e1318_get_wol,
.set_wol = &m88e1318_set_wol,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -974,6 +986,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -987,6 +1001,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -1000,6 +1016,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -1013,6 +1031,8 @@ static struct phy_driver marvell_drivers[] = {
.read_status = &genphy_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
{
@@ -1026,6 +1046,8 @@ static struct phy_driver marvell_drivers[] = {
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
.did_interrupt = &m88e1121_did_interrupt,
+ .resume = &genphy_resume,
+ .suspend = &genphy_suspend,
.driver = { .owner = THIS_MODULE },
},
};
--
1.7.2.5
^ permalink raw reply related
* [PATCH RFC v1 2/7] net: phy: provide phy_resume/phy_suspend helpers
From: Sebastian Hesselbarth @ 2013-11-20 20:21 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Sebastian Hesselbarth, David S. Miller, netdev, linux-arm-kernel,
linux-kernel
In-Reply-To: <1384978913-8052-1-git-send-email-sebastian.hesselbarth@gmail.com>
This adds helper functions to resume and suspend a given phy_device
by calling the corresponding driver callbacks if available.
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/phy/phy_device.c | 19 +++++++++++++++++++
include/linux/phy.h | 2 ++
2 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74630e9..2442895 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -625,6 +625,25 @@ void phy_detach(struct phy_device *phydev)
}
EXPORT_SYMBOL(phy_detach);
+int phy_suspend(struct phy_device *phydev)
+{
+ struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+ if (phydrv->suspend)
+ return phydrv->suspend(phydev);
+ return 0;
+}
+EXPORT_SYMBOL(phy_suspend);
+
+int phy_resume(struct phy_device *phydev)
+{
+ struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
+
+ if (phydrv->resume)
+ return phydrv->resume(phydev);
+ return 0;
+}
+EXPORT_SYMBOL(phy_resume);
/* Generic PHY support and helper functions */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 64ab823..ed0d6d8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -552,6 +552,8 @@ void phy_detach(struct phy_device *phydev);
void phy_start(struct phy_device *phydev);
void phy_stop(struct phy_device *phydev);
int phy_start_aneg(struct phy_device *phydev);
+int phy_suspend(struct phy_device *phydev);
+int phy_resume(struct phy_device *phydev);
int phy_stop_interrupts(struct phy_device *phydev);
--
1.7.2.5
^ permalink raw reply related
* [PATCH RFC v1 3/7] net: phy: resume/suspend PHYs on attach/detach
From: Sebastian Hesselbarth @ 2013-11-20 20:21 UTC (permalink / raw)
To: Sebastian Hesselbarth
Cc: Sebastian Hesselbarth, David S. Miller, netdev, linux-arm-kernel,
linux-kernel
In-Reply-To: <1384978913-8052-1-git-send-email-sebastian.hesselbarth@gmail.com>
This ensures PHYs are resumed on attach and suspended on detach.
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/phy/phy_device.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2442895..a421fc6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -570,6 +570,8 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
if (err)
phy_detach(phydev);
+ phy_resume(phydev);
+
return err;
}
@@ -615,6 +617,7 @@ void phy_detach(struct phy_device *phydev)
{
phydev->attached_dev->phydev = NULL;
phydev->attached_dev = NULL;
+ phy_suspend(phydev);
/* If the device had no specific driver before (i.e. - it
* was using the generic driver), we unbind the device
--
1.7.2.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox