Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v2 1/3] vxlan: silence one build warning
From: David Miller @ 2013-10-29  4:19 UTC (permalink / raw)
  To: zwu.kernel; +Cc: netdev, linux-kernel, stephen, wuzhy
In-Reply-To: <1382940110-10737-1-git-send-email-zwu.kernel@gmail.com>

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Mon, 28 Oct 2013 14:01:48 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> drivers/net/vxlan.c: In function ‘vxlan_sock_add’:
> drivers/net/vxlan.c:2298:11: warning: ‘sock’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> drivers/net/vxlan.c:2275:17: note: ‘sock’ was declared here
>   LD      drivers/net/built-in.o
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Applied to net-next.

^ permalink raw reply

* Re: [PATCH v2 2/3] net, datagram: fix the incorrect comment in zerocopy_sg_from_iovec()
From: David Miller @ 2013-10-29  4:19 UTC (permalink / raw)
  To: zwu.kernel; +Cc: netdev, linux-kernel, stephen, wuzhy
In-Reply-To: <1382940110-10737-2-git-send-email-zwu.kernel@gmail.com>

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Mon, 28 Oct 2013 14:01:49 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Applied to net-next.

^ permalink raw reply

* Re: [PATCH v2 3/3] net, iovec: fix the incorrect comment in memcpy_fromiovecend()
From: David Miller @ 2013-10-29  4:19 UTC (permalink / raw)
  To: zwu.kernel; +Cc: netdev, linux-kernel, stephen, wuzhy
In-Reply-To: <1382940110-10737-3-git-send-email-zwu.kernel@gmail.com>

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Mon, 28 Oct 2013 14:01:50 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Applied to net-next

^ permalink raw reply

* Re: [PATCH] net, mc: fix the incorrect comments in two mc-related functions
From: David Miller @ 2013-10-29  4:19 UTC (permalink / raw)
  To: zwu.kernel; +Cc: netdev, linux-kernel, wuzhy
In-Reply-To: <1382948150-14349-1-git-send-email-zwu.kernel@gmail.com>

From: Zhi Yong Wu <zwu.kernel@gmail.com>
Date: Mon, 28 Oct 2013 16:15:50 +0800

> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Applied to net-next.

^ permalink raw reply

* Re: [PATCH 0/5] r8152 bug fixes
From: David Miller @ 2013-10-29  4:23 UTC (permalink / raw)
  To: hayeswang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1382961494-2272-1-git-send-email-hayeswang@realtek.com>

From: Hayes Wang <hayeswang@realtek.com>
Date: Mon, 28 Oct 2013 19:58:09 +0800

> These could fix some driver issues.
> 
> Hayes Wang (5):
>   net/usb/r8152: fix tx/rx memory overflow
>   net/usb/r8152: make sure the tx checksum setting is correct
>   net/usb/r8152: modify the tx flow
>   net/usb/r8152: fix incorrect type in assignment
>   net/usb/r8152: code adjust

These are not bug fixes, some of them are just cleanups.

It is inappropriate to mix real bug fixes and non-bug fixes into
a patch series.

You must send purely the bug fixes for 'net' tree, and then later
the code cleanups and other changes targetting the 'net-next' tree.

Also, from patch #5:

====================
 -Something else
====================

That is completely unacceptable.  You must say what changes you are
making, the above says nothing to me nor the person reading your
commit messages in the future.

In general, your commit messages are poorly written in that they
contain not enough information for the person trying to understand
your patches at all.

^ permalink raw reply

* Re: [PATCH net V3] xen-netback: use jiffies_64 value to calculate credit timeout
From: David Miller @ 2013-10-29  4:25 UTC (permalink / raw)
  To: wei.liu2
  Cc: xen-devel, netdev, david.vrabel, jbeulich, annie.li, ian.campbell,
	jianhai.luan
In-Reply-To: <1382962077-13406-1-git-send-email-wei.liu2@citrix.com>

From: Wei Liu <wei.liu2@citrix.com>
Date: Mon, 28 Oct 2013 12:07:57 +0000

> time_after_eq() only works if the delta is < MAX_ULONG/2.
> 
> For a 32bit Dom0, if netfront sends packets at a very low rate, the time
> between subsequent calls to tx_credit_exceeded() may exceed MAX_ULONG/2
> and the test for timer_after_eq() will be incorrect. Credit will not be
> replenished and the guest may become unable to send packets (e.g., if
> prior to the long gap, all credit was exhausted).
> 
> Use jiffies_64 variant to mitigate this problem for 32bit Dom0.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Reviewed-by: David Vrabel <david.vrabel@citrix.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply

* Re: [PATCH net-next 2/3] tipc: message reassembly using fragment chain
From: Ying Xue @ 2013-10-29  5:36 UTC (permalink / raw)
  To: David Miller, maloy, jon.maloy
  Cc: paul.gortmaker, erik.hugne, tipc-discussion, netdev
In-Reply-To: <20131028.234953.1884858499661587979.davem@davemloft.net>

On 10/29/2013 11:49 AM, David Miller wrote:
> From: Ying Xue <ying.xue@windriver.com>
> Date: Tue, 29 Oct 2013 09:56:56 +0800
> 
>> Therefore, the best method is to divide tipc_recv_msg() into several
>> smaller functions to simplify the current implementation. But it's not
>> an easy job. Actually I ever tried to do it, but I gave up lastly
>> because I did not find one perfect solution about how to divide it.
> 
> But you're going to have to find a way, this indent level is out of
> control.
> 
> 

By simply adjusting the order of branch blocks, this is what I can figure out the simplest
method to resolve the issue now :)

Please review below change. If it's fine, I will send one formal patch.
(Below change still follows original code logic)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 12c72d2..6c88dbc 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1592,97 +1592,94 @@ void tipc_recv_msg(struct sk_buff *head, struct tipc_bearer *b_ptr)

                /* Now (finally!) process the incoming message */
 protocol_check:
-               if (likely(link_working_working(l_ptr))) {
-                       if (likely(seq_no == mod(l_ptr->next_in_no))) {
-                               l_ptr->next_in_no++;
-                               if (unlikely(l_ptr->oldest_deferred_in))
-                                       head = link_insert_deferred_queue(l_ptr,
-                                                                         head);
-deliver:
-                               if (likely(msg_isdata(msg))) {
-                                       tipc_node_unlock(n_ptr);
-                                       tipc_port_recv_msg(buf);
-                                       continue;
-                               }
-                               switch (msg_user(msg)) {
-                                       int ret;
-                               case MSG_BUNDLER:
-                                       l_ptr->stats.recv_bundles++;
-                                       l_ptr->stats.recv_bundled +=
-                                               msg_msgcnt(msg);
-                                       tipc_node_unlock(n_ptr);
-                                       tipc_link_recv_bundle(buf);
-                                       continue;
-                               case NAME_DISTRIBUTOR:
-                                       n_ptr->bclink.recv_permitted = true;
-                                       tipc_node_unlock(n_ptr);
-                                       tipc_named_recv(buf);
-                                       continue;
-                               case BCAST_PROTOCOL:
-                                       tipc_link_recv_sync(n_ptr, buf);
-                                       tipc_node_unlock(n_ptr);
-                                       continue;
-                               case CONN_MANAGER:
-                                       tipc_node_unlock(n_ptr);
-                                       tipc_port_recv_proto_msg(buf);
-                                       continue;
-                               case MSG_FRAGMENTER:
-                                       l_ptr->stats.recv_fragments++;
-                                       ret = tipc_link_recv_fragment(
-                                               &l_ptr->defragm_buf,
-                                               &buf, &msg);
-                                       if (ret == 1) {
-                                               l_ptr->stats.recv_fragmented++;
-                                               goto deliver;
-                                       }
-                                       if (ret == -1)
-                                               l_ptr->next_in_no--;
-                                       break;
-                               case CHANGEOVER_PROTOCOL:
-                                       type = msg_type(msg);
-                                       if (link_recv_changeover_msg(&l_ptr,
-                                                                    &buf)) {
-                                               msg = buf_msg(buf);
-                                               seq_no = msg_seqno(msg);
-                                               if (type == ORIGINAL_MSG)
-                                                       goto deliver;
-                                               goto protocol_check;
-                                       }
-                                       break;
-                               default:
-                                       kfree_skb(buf);
-                                       buf = NULL;
-                                       break;
-                               }
+               if (unlikely(!link_working_working(l_ptr))) {
+                       if (msg_user(msg) == LINK_PROTOCOL) {
+                               link_recv_proto_msg(l_ptr, buf);
+                               head = link_insert_deferred_queue(l_ptr, head);
+                               tipc_node_unlock(n_ptr);
+                               continue;
+                       }
+
+                       /* Traffic message. Conditionally activate link */
+                       link_state_event(l_ptr, TRAFFIC_MSG_EVT);
+
+                       if (link_working_working(l_ptr)) {
+                               /* Re-insert buffer in front of queue */
+                               buf->next = head;
+                               head = buf;
                                tipc_node_unlock(n_ptr);
-                               tipc_net_route_msg(buf);
                                continue;
                        }
+                       tipc_node_unlock(n_ptr);
+                       goto cont;
+               }
+
+               /* Link is now in state WORKING_WORKING */
+               if (unlikely(seq_no != mod(l_ptr->next_in_no))) {
                        link_handle_out_of_seq_msg(l_ptr, buf);
                        head = link_insert_deferred_queue(l_ptr, head);
                        tipc_node_unlock(n_ptr);
                        continue;
                }
-
-               /* Link is not in state WORKING_WORKING */
-               if (msg_user(msg) == LINK_PROTOCOL) {
-                       link_recv_proto_msg(l_ptr, buf);
+               l_ptr->next_in_no++;
+               if (unlikely(l_ptr->oldest_deferred_in))
                        head = link_insert_deferred_queue(l_ptr, head);
+deliver:
+               if (likely(msg_isdata(msg))) {
                        tipc_node_unlock(n_ptr);
+                       tipc_port_recv_msg(buf);
                        continue;
                }
-
-               /* Traffic message. Conditionally activate link */
-               link_state_event(l_ptr, TRAFFIC_MSG_EVT);
-
-               if (link_working_working(l_ptr)) {
-                       /* Re-insert buffer in front of queue */
-                       buf->next = head;
-                       head = buf;
+               switch (msg_user(msg)) {
+                       int ret;
+               case MSG_BUNDLER:
+                       l_ptr->stats.recv_bundles++;
+                       l_ptr->stats.recv_bundled += msg_msgcnt(msg);
+                       tipc_node_unlock(n_ptr);
+                       tipc_link_recv_bundle(buf);
+                       continue;
+               case NAME_DISTRIBUTOR:
+                       n_ptr->bclink.recv_permitted = true;
+                       tipc_node_unlock(n_ptr);
+                       tipc_named_recv(buf);
+                       continue;
+               case BCAST_PROTOCOL:
+                       tipc_link_recv_sync(n_ptr, buf);
+                       tipc_node_unlock(n_ptr);
+                       continue;
+               case CONN_MANAGER:
                        tipc_node_unlock(n_ptr);
+                       tipc_port_recv_proto_msg(buf);
                        continue;
+               case MSG_FRAGMENTER:
+                       l_ptr->stats.recv_fragments++;
+                       ret = tipc_link_recv_fragment(&l_ptr->defragm_buf,
+                                                     &buf, &msg);
+                       if (ret == 1) {
+                               l_ptr->stats.recv_fragmented++;
+                               goto deliver;
+                       }
+                       if (ret == -1)
+                               l_ptr->next_in_no--;
+                       break;
+               case CHANGEOVER_PROTOCOL:
+                       type = msg_type(msg);
+                       if (link_recv_changeover_msg(&l_ptr, &buf)) {
+                               msg = buf_msg(buf);
+                               seq_no = msg_seqno(msg);
+                               if (type == ORIGINAL_MSG)
+                                       goto deliver;
+                               goto protocol_check;
+                       }
+                       break;
+               default:
+                       kfree_skb(buf);
+                       buf = NULL;
+                       break;
                }
                tipc_node_unlock(n_ptr);
+               tipc_net_route_msg(buf);
+               continue;
 cont:
                kfree_skb(buf);
        }

Regards,
Ying

^ permalink raw reply related

* Re: [PATCH 00/19] Enable various Renesas drivers on all ARM platforms
From: Simon Horman @ 2013-10-29  6:04 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-fbdev, Wolfram Sang, Linus Walleij, Guennadi Liakhovetski,
	Thierry Reding, linux-mtd, linux-i2c, Vinod Koul, Joerg Roedel,
	linux-sh, Magnus Damm, Eduardo Valentin, Tomi Valkeinen,
	linux-serial, linux-input, Zhang Rui, Chris Ball,
	Jean-Christophe Plagniol-Villard, linux-media, linux-pwm,
	Samuel Ortiz, linux-pm, Ian Molton, Mark Brown, linux-arm-kernel,
	Ser
In-Reply-To: <1383004027-25036-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com>

On Tue, Oct 29, 2013 at 12:46:48AM +0100, Laurent Pinchart wrote:
> Hello,
> 
> This patch series, based on v3.12-rc7, prepares various Renesas drivers
> for migration to multiplatform kernels by enabling their compilation or
> otherwise fixing them on all ARM platforms. The patches are pretty
> straightforward and are described in their commit message.
> 
> I'd like to get all these patches merged in v3.14. As they will need to go
> through their respective subsystems' trees, I would appreciate if all
> maintainers involved could notify me when they merge patches from this series
> in their tree to help me tracking the merge status. I don't plan to send pull
> requests individually for these patches, and I will repost patches
> individually if changes are requested during review.
> 
> If you believe the issue should be solved in a different way (for instance by
> removing the architecture dependency completely) please reply to the cover
> letter to let other maintainers chime in.

I think this is a step in a good direction.
However, I think it would be even better if the architecture dependency was
removed completely.

> 
> Cc: Chris Ball <cjb@laptop.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: dmaengine@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Eduardo Valentin <eduardo.valentin@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> Cc: Ian Molton <ian@mnementh.co.uk>
> Cc: iommu@lists.linux-foundation.org
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-i2c@vger.kernel.org
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-media@vger.kernel.org
> Cc: linux-mmc@vger.kernel.org
> Cc: linux-mtd@lists.infradead.org
> Cc: linux-pm@vger.kernel.org
> Cc: linux-pwm@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: linux-spi@vger.kernel.org
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com>
> Cc: netdev@vger.kernel.org
> Cc: Samuel Ortiz <samuel@sortiz.org>
> Cc: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> Cc: Zhang Rui <rui.zhang@intel.com>
> 
> Laurent Pinchart (19):
>   serial: sh-sci: Enable the driver on all ARM platforms
>   DMA: shdma: Enable the driver on all ARM platforms
>   i2c: sh_mobile: Enable the driver on all ARM platforms
>   input: sh_keysc: Enable the driver on all ARM platforms
>   iommu: shmobile: Enable the driver on all ARM platforms
>   i2c: rcar: Enable the driver on all ARM platforms
>   v4l: sh_vou: Enable the driver on all ARM platforms
>   mmc: sdhi: Enable the driver on all ARM platforms
>   mmc: sh_mmcif: Enable the driver on all ARM platforms
>   mtd: sh_flctl: Enable the driver on all ARM platforms
>   net: sh_eth: Set receive alignment correctly on all ARM platforms
>   irda: sh_irda: Enable the driver on all ARM platforms
>   pinctrl: sh-pfc: Enable the driver on all ARM platforms
>   pwm: pwm-renesas-tpu: Enable the driver on all ARM platforms
>   sh: intc: Enable the driver on all ARM platforms
>   spi: sh_msiof: Enable the driver on all ARM platforms
>   spi: sh_hspi: Enable the driver on all ARM platforms
>   thermal: rcar-thermal: Enable the driver on all ARM platforms
>   fbdev: sh-mobile-lcdcfb: Enable the driver on all ARM platforms
> 
>  drivers/dma/sh/Kconfig                | 2 +-
>  drivers/dma/sh/shdmac.c               | 6 +++---
>  drivers/i2c/busses/Kconfig            | 4 ++--
>  drivers/input/keyboard/Kconfig        | 2 +-
>  drivers/iommu/Kconfig                 | 2 +-
>  drivers/media/platform/Kconfig        | 2 +-
>  drivers/mmc/host/Kconfig              | 4 ++--
>  drivers/mmc/host/tmio_mmc_dma.c       | 2 +-
>  drivers/mtd/nand/Kconfig              | 2 +-
>  drivers/net/ethernet/renesas/sh_eth.c | 2 +-
>  drivers/net/ethernet/renesas/sh_eth.h | 2 +-
>  drivers/net/irda/Kconfig              | 2 +-
>  drivers/pinctrl/Makefile              | 2 +-
>  drivers/pinctrl/sh-pfc/Kconfig        | 2 +-
>  drivers/pwm/Kconfig                   | 2 +-
>  drivers/sh/intc/Kconfig               | 2 +-
>  drivers/spi/Kconfig                   | 4 ++--
>  drivers/thermal/Kconfig               | 2 +-
>  drivers/tty/serial/Kconfig            | 2 +-
>  drivers/video/Kconfig                 | 6 +++---
>  20 files changed, 27 insertions(+), 27 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply

* Re: [PATCH net-next] virtio_net: migrate mergeable rx buffers to page frag allocators
From: Jason Wang @ 2013-10-29  6:27 UTC (permalink / raw)
  To: Michael Dalton, David S. Miller
  Cc: Michael S. Tsirkin, netdev, Daniel Borkmann, virtualization,
	Eric Dumazet
In-Reply-To: <1383000258-1458-1-git-send-email-mwdalton@google.com>

On 10/29/2013 06:44 AM, Michael Dalton wrote:
> The virtio_net driver's mergeable receive buffer allocator
> uses 4KB packet buffers. For MTU-sized traffic, SKB truesize
> is > 4KB but only ~1500 bytes of the buffer is used to store
> packet data, reducing the effective TCP window size
> substantially. This patch addresses the performance concerns
> with mergeable receive buffers by allocating MTU-sized packet
> buffers using page frag allocators. If more than MAX_SKB_FRAGS
> buffers are needed, the SKB frag_list is used.
>
> Signed-off-by: Michael Dalton <mwdalton@google.com>

Do you have any perf numberf of this patch? Have a glance of the patch,
looks like it will hurt the performance of large GSO packet. Always
allocating 1500 bytes mean a virtqueue can only hold about 4 full size
gso packets, and frag list will introduce extra overheads on skb
allocation. I just test it on my desktop, performance of full size gso
packet drops about 10%.

Mergeable buffer is a good balance between performance and the space it
occupies. If you just want a ~1500 bytes packet to be received, you can
just disable the mergeable buffer and just use small packet.

Alternatively, a simpler way is just use build_skb() and head frag to
build the skb directly on the page for medium size packets (small
packets were still copied) like following patch (may need more work
since vnet header is too short for NET_SKB_PAD). This can reduce the
truesize while keep the performance for large GSO packet.

---
 drivers/net/virtio_net.c |   48
++++++++++++++++++++++++++++++---------------
 1 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3c23fdc..4c7ad89 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -239,16 +239,12 @@ static struct sk_buff *page_to_skb(struct
receive_queue *rq,
     struct skb_vnet_hdr *hdr;
     unsigned int copy, hdr_len, offset;
     char *p;
+    int skb_size = SKB_DATA_ALIGN(len) +
+               SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+    bool frag;
 
     p = page_address(page);
 
-    /* copy small packet so we can reuse these pages for small data */
-    skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
-    if (unlikely(!skb))
-        return NULL;
-
-    hdr = skb_vnet_hdr(skb);
-
     if (vi->mergeable_rx_bufs) {
         hdr_len = sizeof hdr->mhdr;
         offset = hdr_len;
@@ -257,18 +253,38 @@ static struct sk_buff *page_to_skb(struct
receive_queue *rq,
         offset = sizeof(struct padded_vnet_hdr);
     }
 
-    memcpy(hdr, p, hdr_len);
-
     len -= hdr_len;
     p += offset;
 
-    copy = len;
-    if (copy > skb_tailroom(skb))
-        copy = skb_tailroom(skb);
-    memcpy(skb_put(skb, copy), p, copy);
+    frag = (len > GOOD_COPY_LEN) && (skb_size <= PAGE_SIZE) &&
+        vi->mergeable_rx_bufs;
+    if (frag) {
+        skb = build_skb(page_address(page), skb_size);
+        if (unlikely(!skb))
+            return NULL;
+
+        skb_reserve(skb, offset);
+        skb_put(skb, len);
+        len = 0;
+    } else {
+        /* copy small packet so we can reuse these pages for small data
+         */
+        skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
+        if (unlikely(!skb))
+            return NULL;
+
+        copy = len;
+        if (copy > skb_tailroom(skb))
+            copy = skb_tailroom(skb);
+        memcpy(skb_put(skb, copy), p, copy);
+
+        len -= copy;
+        offset += copy;
+    }
+
+    hdr = skb_vnet_hdr(skb);
 
-    len -= copy;
-    offset += copy;
+    memcpy(hdr, page_address(page), hdr_len);
 
     /*
      * Verify that we can indeed put this data into a skb.
@@ -288,7 +304,7 @@ static struct sk_buff *page_to_skb(struct
receive_queue *rq,
         offset = 0;
     }
 
-    if (page)
+    if (page && !frag)
         give_pages(rq, page);
 
     return skb;
-- 
1.7.1

^ permalink raw reply related

* Re: [RFC TRY#2][PATCH] bgmac: pass received packet to the netif instead of copying it
From: Nathan Hintz @ 2013-10-29  6:50 UTC (permalink / raw)
  To: OpenWrt Development List; +Cc: netdev, David S. Miller
In-Reply-To: <1382982142-16876-1-git-send-email-zajec5@gmail.com>

On Mon, 28 Oct 2013 18:42:22 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

Hi:

A few questions/comments inline...

Nathan

> Copying whole packets with skb_copy_from_linear_data_offset is a pretty
> bad idea. CPU was spending time in __copy_user_common and network
> performance was lower. With the new solution iperf-measured speed
> increased from 116Mb/s to 134Mb/s.
> 
> Another way to improve performance could be switching to build_skb. It
> is cache-specific, so will require testing of various devices.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  drivers/net/ethernet/broadcom/bgmac.c |   71 ++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 6b7541f..fde9a11 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -307,7 +307,6 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  		struct device *dma_dev = bgmac->core->dma_dev;
>  		struct bgmac_slot_info *slot = &ring->slots[ring->start];
>  		struct sk_buff *skb = slot->skb;
> -		struct sk_buff *new_skb;
>  		struct bgmac_rx_header *rx;
>  		u16 len, flags;
>  
> @@ -320,38 +319,56 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>  		len = le16_to_cpu(rx->len);
>  		flags = le16_to_cpu(rx->flags);
>  
> -		/* Check for poison and drop or pass the packet */
> -		if (len == 0xdead && flags == 0xbeef) {
> -			bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
> -				  ring->start);
> -		} else {
> +		do {

"old_skb" duplicates "skb" stored above, can that one be used (renamed) instead of creating a new one here?

> +			struct sk_buff *old_skb = slot->skb;
> +			dma_addr_t old_dma_addr = slot->dma_addr;
> +			int err;
> +
> +			/* Check for poison and drop or pass the packet */
> +			if (len == 0xdead && flags == 0xbeef) {
> +				bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
> +					  ring->start);

Nothing in the buffer has been changed by the cpu yet, so is this sync necessary?

> +				dma_sync_single_for_device(dma_dev,
> +							   slot->dma_addr,
> +							   BGMAC_RX_BUF_SIZE,
> +							   DMA_FROM_DEVICE);
> +				break;
> +			}
> +
>  			/* Omit CRC. */
>  			len -= ETH_FCS_LEN;
>  
> -			new_skb = netdev_alloc_skb_ip_align(bgmac->net_dev, len);
> -			if (new_skb) {
> -				skb_put(new_skb, len);
> -				skb_copy_from_linear_data_offset(skb, BGMAC_RX_FRAME_OFFSET,
> -								 new_skb->data,
> -								 len);
> -				skb_checksum_none_assert(skb);
> -				new_skb->protocol =
> -					eth_type_trans(new_skb, bgmac->net_dev);
> -				netif_receive_skb(new_skb);
> -				handled++;
> -			} else {
> -				bgmac->net_dev->stats.rx_dropped++;
> -				bgmac_err(bgmac, "Allocation of skb for copying packet failed!\n");
> +			/* Prepare new skb as replacement */
> +			err = bgmac_dma_rx_skb_for_slot(bgmac, slot);
> +			if (err) {

I've sent a separate patch against "bgmac_dma_rx_skb_for_slot" to not corrupt the slot at all if an
error occurs (skb alloc or dma mapping), and free the skb that was allocated if a dma mapping error
occurs.  Assuming that patch is accepted, then the following two lines would not be needed.
With "bgmac_dma_rx_skb_for_slot" as it currently exists, this would leak an skb for a dma mapping
error (this was pre-existing to the changes in this patch).

> +				slot->skb = old_skb;
> +				slot->dma_addr = old_dma_addr;
> +
> +				/* Poison the old skb */
> +				rx->len = cpu_to_le16(0xdead);
> +				rx->flags = cpu_to_le16(0xbeef);
> +
> +				dma_sync_single_for_device(dma_dev,
> +							   slot->dma_addr,
> +							   BGMAC_RX_BUF_SIZE,
> +							   DMA_FROM_DEVICE);
> +				break;
>  			}
> +			bgmac_dma_rx_setup_desc(bgmac, ring, ring->start);
>  
> -			/* Poison the old skb */
> -			rx->len = cpu_to_le16(0xdead);
> -			rx->flags = cpu_to_le16(0xbeef);
> -		}
> +			/* Unmap old skb, we'll pass it to the netfif */
> +			dma_unmap_single(dma_dev, old_dma_addr,
> +						BGMAC_RX_BUF_SIZE,
> +						DMA_FROM_DEVICE);
> +
> +			skb_put(skb, BGMAC_RX_FRAME_OFFSET + len);
> +			skb_pull(skb, BGMAC_RX_FRAME_OFFSET);
>  
> -		/* Make it back accessible to the hardware */
> -		dma_sync_single_for_device(dma_dev, slot->dma_addr,
> -					   BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
> +			skb_checksum_none_assert(skb);
> +			skb->protocol = eth_type_trans(skb, bgmac->net_dev);
> +			netif_receive_skb(skb);
> +			handled++;
> +		} while (0);
>  
>  		if (++ring->start >= BGMAC_RX_RING_SLOTS)
>  			ring->start = 0;



-- 
Nathan
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

^ permalink raw reply

* [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
From: Nathan Hintz @ 2013-10-29  6:44 UTC (permalink / raw)
  To: netdev; +Cc: zajec5

Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
skb alloc and dma mapping are successful; and free the newly allocated
skb if a dma mapping error occurs.  This will prevent an skb leak upon
returning when an error occurs.

Signed-off-by: Nathan Hintz <nlhintz@hotmail.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 7eca5a1..11e5d8d 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -252,25 +252,33 @@ static int bgmac_dma_rx_skb_for_slot(struct bgmac *bgmac,
 				     struct bgmac_slot_info *slot)
 {
 	struct device *dma_dev = bgmac->core->dma_dev;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
 	struct bgmac_rx_header *rx;
 
 	/* Alloc skb */
-	slot->skb = netdev_alloc_skb(bgmac->net_dev, BGMAC_RX_BUF_SIZE);
-	if (!slot->skb)
+	skb = netdev_alloc_skb(bgmac->net_dev, BGMAC_RX_BUF_SIZE);
+	if (!skb)
 		return -ENOMEM;
 
 	/* Poison - if everything goes fine, hardware will overwrite it */
-	rx = (struct bgmac_rx_header *)slot->skb->data;
+	rx = (struct bgmac_rx_header *)skb->data;
 	rx->len = cpu_to_le16(0xdead);
 	rx->flags = cpu_to_le16(0xbeef);
 
 	/* Map skb for the DMA */
-	slot->dma_addr = dma_map_single(dma_dev, slot->skb->data,
-					BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
-	if (dma_mapping_error(dma_dev, slot->dma_addr)) {
+	dma_addr = dma_map_single(dma_dev, skb->data,
+				  BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(dma_dev, dma_addr)) {
 		bgmac_err(bgmac, "DMA mapping error\n");
+		dev_kfree_skb(skb);
 		return -ENOMEM;
 	}
+
+	/* Update the slot */
+	slot->skb = skb;
+	slot->dma_addr = dma_addr;
+
 	if (slot->dma_addr & 0xC0000000)
 		bgmac_warn(bgmac, "DMA address using 0xC0000000 bit(s), it may need translation trick\n");
 
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
From: Rafał Miłecki @ 2013-10-29  6:52 UTC (permalink / raw)
  To: Nathan Hintz; +Cc: Network Development
In-Reply-To: <BLU0-SMTP119F7A71B34291D1CC2486BAC090@phx.gbl>

2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
> skb alloc and dma mapping are successful; and free the newly allocated
> skb if a dma mapping error occurs.  This will prevent an skb leak upon
> returning when an error occurs.

In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
(and freeing everything), but with your patch code is simpler to
understand, so I'm OK with that.

Acked-by: Rafał Miłecki <zajec5@gmail.com>

-- 
Rafał

^ permalink raw reply

* Re: [OpenWrt-Devel] [RFC TRY#2][PATCH] bgmac: pass received packet to the netif instead of copying it
From: Rafał Miłecki @ 2013-10-29  6:59 UTC (permalink / raw)
  To: Nathan Hintz
  Cc: OpenWrt Development List, Network Development, David S. Miller
In-Reply-To: <BLU0-SMTP122B5D616AD665A29B9DDEFAC090@phx.gbl>

2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> On Mon, 28 Oct 2013 18:42:22 +0100
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
> Hi:
>
> A few questions/comments inline...
>
> Nathan
>
>> Copying whole packets with skb_copy_from_linear_data_offset is a pretty
>> bad idea. CPU was spending time in __copy_user_common and network
>> performance was lower. With the new solution iperf-measured speed
>> increased from 116Mb/s to 134Mb/s.
>>
>> Another way to improve performance could be switching to build_skb. It
>> is cache-specific, so will require testing of various devices.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c |   71 ++++++++++++++++++++-------------
>>  1 file changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 6b7541f..fde9a11 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -307,7 +307,6 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>               struct device *dma_dev = bgmac->core->dma_dev;
>>               struct bgmac_slot_info *slot = &ring->slots[ring->start];
>>               struct sk_buff *skb = slot->skb;
>> -             struct sk_buff *new_skb;
>>               struct bgmac_rx_header *rx;
>>               u16 len, flags;
>>
>> @@ -320,38 +319,56 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>               len = le16_to_cpu(rx->len);
>>               flags = le16_to_cpu(rx->flags);
>>
>> -             /* Check for poison and drop or pass the packet */
>> -             if (len == 0xdead && flags == 0xbeef) {
>> -                     bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
>> -                               ring->start);
>> -             } else {
>> +             do {
>
> "old_skb" duplicates "skb" stored above, can that one be used (renamed) instead of creating a new one here?

I've focused on clean code too much. That won't be needed anyway when
I rebase my patch on top of yours.


>> +                     struct sk_buff *old_skb = slot->skb;
>> +                     dma_addr_t old_dma_addr = slot->dma_addr;
>> +                     int err;
>> +
>> +                     /* Check for poison and drop or pass the packet */
>> +                     if (len == 0xdead && flags == 0xbeef) {
>> +                             bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n",
>> +                                       ring->start);
>> +                             dma_sync_single_for_device(dma_dev,
>> +                                                        slot->dma_addr,
>> +                                                        BGMAC_RX_BUF_SIZE,
>> +                                                        DMA_FROM_DEVICE);
>
> Nothing in the buffer has been changed by the cpu yet, so is this sync necessary?

I've moved your comment a line below, so it comments the code *above*.

I'm using LDD3 to understand DMA and it contains this explanation:

> void dma_sync_single_for_cpu(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);
>
> This function should be called before the processor accesses a streaming DMA buffer. Once the call has been made, the CPU “owns” the DMA buffer and can work with it as needed. Before the device accesses the buffer, however, ownership should be transferred back to it with:
>
> void dma_sync_single_for_device(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);

So even if I didn't change anything in the buffer, I believe we still
need to "sync" it back to make it accessible to the hardware.


> I've sent a separate patch against "bgmac_dma_rx_skb_for_slot" to not corrupt the slot at all if an
> error occurs (skb alloc or dma mapping), and free the skb that was allocated if a dma mapping error
> occurs.  Assuming that patch is accepted, then the following two lines would not be needed.
> With "bgmac_dma_rx_skb_for_slot" as it currently exists, this would leak an skb for a dma mapping
> error (this was pre-existing to the changes in this patch).

Thanks, I'll rebase my patch.

^ permalink raw reply

* Re: [PATCH 03/16] wl1251: add sysfs interface for bluetooth coexistence mode configuration
From: Luca Coelho @ 2013-10-29  7:09 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Pali Rohár, John W. Linville, Johannes Berg, David S. Miller,
	linux-wireless, netdev, linux-kernel, freemangordon,
	aaro.koskinen, pavel, sre, joni.lapilainen, David Gnedt
In-Reply-To: <1383003587.3779.49.camel@bwh-desktop.uk.level5networks.com>

On Mon, 2013-10-28 at 23:39 +0000, Ben Hutchings wrote:
> On Sat, 2013-10-26 at 22:34 +0200, Pali Rohár wrote:
> > From: David Gnedt <david.gnedt@davizone.at>
> > 
> > Port the bt_coex_mode sysfs interface from wl1251 driver version included
> > in the Maemo Fremantle kernel to allow bt-coexistence mode configuration.
> > This enables userspace applications to set one of the modes
> > WL1251_BT_COEX_OFF, WL1251_BT_COEX_ENABLE and WL1251_BT_COEX_MONOAUDIO.
> > The default mode is WL1251_BT_COEX_OFF.
> > It should be noted that this driver always enabled bt-coexistence before
> > and enabled bt-coexistence directly affects the receiving performance,
> > rendering it unusable in some low-signal situations. Especially monitor
> > mode is affected very badly with bt-coexistence enabled.
> [...]
> 
> This should be implemented consistently with other drivers:
> 
> drivers/net/wireless/ath/ath9k/htc_drv_init.c:module_param_named(btcoex_enable, ath9k_htc_btcoex_enable, int, 0444);
> drivers/net/wireless/ath/ath9k/init.c:module_param_named(btcoex_enable, ath9k_btcoex_enable, int, 0444);
> drivers/net/wireless/b43/main.c:module_param_named(btcoex, modparam_btcoex, int, 0444);
> drivers/net/wireless/ipw2x00/ipw2200.c:module_param(bt_coexist, int, 0444);
> drivers/net/wireless/iwlegacy/common.c:module_param(bt_coex_active, bool, S_IRUGO);
> drivers/net/wireless/iwlwifi/iwl-drv.c:module_param_named(bt_coex_active, iwlwifi_mod_params.bt_coex_active,
> drivers/net/wireless/ti/wlcore/sysfs.c:static DEVICE_ATTR(bt_coex_state, S_IRUGO | S_IWUSR,
> 
> Oh, hmm, I see a problem here.

With so many drivers doing the same thing, isn't it about time to add
this to nl80211?

--
Luca.

^ permalink raw reply

* [PATCH net] virtio-net: correctly handle cpu hotplug notifier during resuming
From: Jason Wang @ 2013-10-29  7:11 UTC (permalink / raw)
  To: rusty, mst, virtualization, netdev, linux-kernel

commit 3ab098df35f8b98b6553edc2e40234af512ba877 (virtio-net: don't respond to
cpu hotplug notifier if we're not ready) tries to bypass the cpu hotplug
notifier by checking the config_enable and does nothing is it was false. So it
need to try to hold the config_lock mutex which may happen in atomic
environment which leads the following warnings:

[  622.944441] CPU0 attaching NULL sched-domain.
[  622.944446] CPU1 attaching NULL sched-domain.
[  622.944485] CPU0 attaching NULL sched-domain.
[  622.950795] BUG: sleeping function called from invalid context at kernel/mutex.c:616
[  622.950796] in_atomic(): 1, irqs_disabled(): 1, pid: 10, name: migration/1
[  622.950796] no locks held by migration/1/10.
[  622.950798] CPU: 1 PID: 10 Comm: migration/1 Not tainted 3.12.0-rc5-wl-01249-gb91e82d #317
[  622.950799] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[  622.950802]  0000000000000000 ffff88001d42dba0 ffffffff81a32f22 ffff88001bfb9c70
[  622.950803]  ffff88001d42dbb0 ffffffff810edb02 ffff88001d42dc38 ffffffff81a396ed
[  622.950805]  0000000000000046 ffff88001d42dbe8 ffffffff810e861d 0000000000000000
[  622.950805] Call Trace:
[  622.950810]  [<ffffffff81a32f22>] dump_stack+0x54/0x74
[  622.950815]  [<ffffffff810edb02>] __might_sleep+0x112/0x114
[  622.950817]  [<ffffffff81a396ed>] mutex_lock_nested+0x3c/0x3c6
[  622.950818]  [<ffffffff810e861d>] ? up+0x39/0x3e
[  622.950821]  [<ffffffff8153ea7c>] ? acpi_os_signal_semaphore+0x21/0x2d
[  622.950824]  [<ffffffff81565ed1>] ? acpi_ut_release_mutex+0x5e/0x62
[  622.950828]  [<ffffffff816d04ec>] virtnet_cpu_callback+0x33/0x87
[  622.950830]  [<ffffffff81a42576>] notifier_call_chain+0x3c/0x5e
[  622.950832]  [<ffffffff810e86a8>] __raw_notifier_call_chain+0xe/0x10
[  622.950835]  [<ffffffff810c5556>] __cpu_notify+0x20/0x37
[  622.950836]  [<ffffffff810c5580>] cpu_notify+0x13/0x15
[  622.950838]  [<ffffffff81a237cd>] take_cpu_down+0x27/0x3a
[  622.950841]  [<ffffffff81136289>] stop_machine_cpu_stop+0x93/0xf1
[  622.950842]  [<ffffffff81136167>] cpu_stopper_thread+0xa0/0x12f
[  622.950844]  [<ffffffff811361f6>] ? cpu_stopper_thread+0x12f/0x12f
[  622.950847]  [<ffffffff81119710>] ? lock_release_holdtime.part.7+0xa3/0xa8
[  622.950848]  [<ffffffff81135e4b>] ? cpu_stop_should_run+0x3f/0x47
[  622.950850]  [<ffffffff810ea9b0>] smpboot_thread_fn+0x1c5/0x1e3
[  622.950852]  [<ffffffff810ea7eb>] ? lg_global_unlock+0x67/0x67
[  622.950854]  [<ffffffff810e36b7>] kthread+0xd8/0xe0
[  622.950857]  [<ffffffff81a3bfad>] ? wait_for_common+0x12f/0x164
[  622.950859]  [<ffffffff810e35df>] ? kthread_create_on_node+0x124/0x124
[  622.950861]  [<ffffffff81a45ffc>] ret_from_fork+0x7c/0xb0
[  622.950862]  [<ffffffff810e35df>] ? kthread_create_on_node+0x124/0x124
[  622.950876] smpboot: CPU 1 is now offline
[  623.194556] SMP alternatives: lockdep: fixing up alternatives
[  623.194559] smpboot: Booting Node 0 Processor 1 APIC 0x1
...

A correct fix is to unregister the hotcpu notifier during restore and register a
new one in resume.

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Tested-by: Fengguang Wu <fengguang.wu@intel.com>
Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
This patch is needed for 3.8 and above.
---
 drivers/net/virtio_net.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9fbdfcd..bbc9cb8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1118,11 +1118,6 @@ static int virtnet_cpu_callback(struct notifier_block *nfb,
 {
 	struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb);
 
-	mutex_lock(&vi->config_lock);
-
-	if (!vi->config_enable)
-		goto done;
-
 	switch(action & ~CPU_TASKS_FROZEN) {
 	case CPU_ONLINE:
 	case CPU_DOWN_FAILED:
@@ -1136,8 +1131,6 @@ static int virtnet_cpu_callback(struct notifier_block *nfb,
 		break;
 	}
 
-done:
-	mutex_unlock(&vi->config_lock);
 	return NOTIFY_OK;
 }
 
@@ -1699,6 +1692,8 @@ static int virtnet_freeze(struct virtio_device *vdev)
 	struct virtnet_info *vi = vdev->priv;
 	int i;
 
+	unregister_hotcpu_notifier(&vi->nb);
+
 	/* Prevent config work handler from accessing the device */
 	mutex_lock(&vi->config_lock);
 	vi->config_enable = false;
@@ -1747,6 +1742,10 @@ static int virtnet_restore(struct virtio_device *vdev)
 	virtnet_set_queues(vi, vi->curr_queue_pairs);
 	rtnl_unlock();
 
+	err = register_hotcpu_notifier(&vi->nb);
+	if (err)
+		return err;
+
 	return 0;
 }
 #endif
-- 
1.8.1.2

^ permalink raw reply related

* Re: [PATCH net-next] virtio_net: migrate mergeable rx buffers to page frag allocators
From: Daniel Borkmann @ 2013-10-29  7:30 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Michael Dalton, David S. Miller, netdev, Eric Dumazet,
	Rusty Russell, Michael S. Tsirkin, virtualization,
	Francesco Fusco
In-Reply-To: <1383002389.4344.7.camel@edumazet-glaptop.roam.corp.google.com>

On 10/29/2013 12:19 AM, Eric Dumazet wrote:
> On Mon, 2013-10-28 at 15:44 -0700, Michael Dalton wrote:
>> The virtio_net driver's mergeable receive buffer allocator
>> uses 4KB packet buffers. For MTU-sized traffic, SKB truesize
>> is > 4KB but only ~1500 bytes of the buffer is used to store
>> packet data, reducing the effective TCP window size
>> substantially. This patch addresses the performance concerns
>> with mergeable receive buffers by allocating MTU-sized packet
>> buffers using page frag allocators. If more than MAX_SKB_FRAGS
>> buffers are needed, the SKB frag_list is used.
>>
>> Signed-off-by: Michael Dalton <mwdalton@google.com>
>> ---
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Daniel & Francesco, this should address the performance problem you
> tried to address with ("tcp: rcvbuf autotuning improvements")

That's awesome, thanks everyone !

> ( http://www.spinics.net/lists/netdev/msg252642.html )
>
> Thanks !

^ permalink raw reply

* Re: [PATCH net] virtio-net: correctly handle cpu hotplug notifier during resuming
From: Michael S. Tsirkin @ 2013-10-29  7:36 UTC (permalink / raw)
  To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1383030667-14343-1-git-send-email-jasowang@redhat.com>

On Tue, Oct 29, 2013 at 03:11:07PM +0800, Jason Wang wrote:
> commit 3ab098df35f8b98b6553edc2e40234af512ba877 (virtio-net: don't respond to
> cpu hotplug notifier if we're not ready) tries to bypass the cpu hotplug
> notifier by checking the config_enable and does nothing is it was false. So it
> need to try to hold the config_lock mutex which may happen in atomic
> environment which leads the following warnings:
> 
> [  622.944441] CPU0 attaching NULL sched-domain.
> [  622.944446] CPU1 attaching NULL sched-domain.
> [  622.944485] CPU0 attaching NULL sched-domain.
> [  622.950795] BUG: sleeping function called from invalid context at kernel/mutex.c:616
> [  622.950796] in_atomic(): 1, irqs_disabled(): 1, pid: 10, name: migration/1
> [  622.950796] no locks held by migration/1/10.
> [  622.950798] CPU: 1 PID: 10 Comm: migration/1 Not tainted 3.12.0-rc5-wl-01249-gb91e82d #317
> [  622.950799] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [  622.950802]  0000000000000000 ffff88001d42dba0 ffffffff81a32f22 ffff88001bfb9c70
> [  622.950803]  ffff88001d42dbb0 ffffffff810edb02 ffff88001d42dc38 ffffffff81a396ed
> [  622.950805]  0000000000000046 ffff88001d42dbe8 ffffffff810e861d 0000000000000000
> [  622.950805] Call Trace:
> [  622.950810]  [<ffffffff81a32f22>] dump_stack+0x54/0x74
> [  622.950815]  [<ffffffff810edb02>] __might_sleep+0x112/0x114
> [  622.950817]  [<ffffffff81a396ed>] mutex_lock_nested+0x3c/0x3c6
> [  622.950818]  [<ffffffff810e861d>] ? up+0x39/0x3e
> [  622.950821]  [<ffffffff8153ea7c>] ? acpi_os_signal_semaphore+0x21/0x2d
> [  622.950824]  [<ffffffff81565ed1>] ? acpi_ut_release_mutex+0x5e/0x62
> [  622.950828]  [<ffffffff816d04ec>] virtnet_cpu_callback+0x33/0x87
> [  622.950830]  [<ffffffff81a42576>] notifier_call_chain+0x3c/0x5e
> [  622.950832]  [<ffffffff810e86a8>] __raw_notifier_call_chain+0xe/0x10
> [  622.950835]  [<ffffffff810c5556>] __cpu_notify+0x20/0x37
> [  622.950836]  [<ffffffff810c5580>] cpu_notify+0x13/0x15
> [  622.950838]  [<ffffffff81a237cd>] take_cpu_down+0x27/0x3a
> [  622.950841]  [<ffffffff81136289>] stop_machine_cpu_stop+0x93/0xf1
> [  622.950842]  [<ffffffff81136167>] cpu_stopper_thread+0xa0/0x12f
> [  622.950844]  [<ffffffff811361f6>] ? cpu_stopper_thread+0x12f/0x12f
> [  622.950847]  [<ffffffff81119710>] ? lock_release_holdtime.part.7+0xa3/0xa8
> [  622.950848]  [<ffffffff81135e4b>] ? cpu_stop_should_run+0x3f/0x47
> [  622.950850]  [<ffffffff810ea9b0>] smpboot_thread_fn+0x1c5/0x1e3
> [  622.950852]  [<ffffffff810ea7eb>] ? lg_global_unlock+0x67/0x67
> [  622.950854]  [<ffffffff810e36b7>] kthread+0xd8/0xe0
> [  622.950857]  [<ffffffff81a3bfad>] ? wait_for_common+0x12f/0x164
> [  622.950859]  [<ffffffff810e35df>] ? kthread_create_on_node+0x124/0x124
> [  622.950861]  [<ffffffff81a45ffc>] ret_from_fork+0x7c/0xb0
> [  622.950862]  [<ffffffff810e35df>] ? kthread_create_on_node+0x124/0x124
> [  622.950876] smpboot: CPU 1 is now offline
> [  623.194556] SMP alternatives: lockdep: fixing up alternatives
> [  623.194559] smpboot: Booting Node 0 Processor 1 APIC 0x1
> ...
> 
> A correct fix is to unregister the hotcpu notifier during restore and register a
> new one in resume.
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Tested-by: Fengguang Wu <fengguang.wu@intel.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

BTW there's a new Fixes: tag, you might want to use it
in the future.

> ---
> This patch is needed for 3.8 and above.
> ---
>  drivers/net/virtio_net.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9fbdfcd..bbc9cb8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1118,11 +1118,6 @@ static int virtnet_cpu_callback(struct notifier_block *nfb,
>  {
>  	struct virtnet_info *vi = container_of(nfb, struct virtnet_info, nb);
>  
> -	mutex_lock(&vi->config_lock);
> -
> -	if (!vi->config_enable)
> -		goto done;
> -
>  	switch(action & ~CPU_TASKS_FROZEN) {
>  	case CPU_ONLINE:
>  	case CPU_DOWN_FAILED:
> @@ -1136,8 +1131,6 @@ static int virtnet_cpu_callback(struct notifier_block *nfb,
>  		break;
>  	}
>  
> -done:
> -	mutex_unlock(&vi->config_lock);
>  	return NOTIFY_OK;
>  }
>  
> @@ -1699,6 +1692,8 @@ static int virtnet_freeze(struct virtio_device *vdev)
>  	struct virtnet_info *vi = vdev->priv;
>  	int i;
>  
> +	unregister_hotcpu_notifier(&vi->nb);
> +
>  	/* Prevent config work handler from accessing the device */
>  	mutex_lock(&vi->config_lock);
>  	vi->config_enable = false;
> @@ -1747,6 +1742,10 @@ static int virtnet_restore(struct virtio_device *vdev)
>  	virtnet_set_queues(vi, vi->curr_queue_pairs);
>  	rtnl_unlock();
>  
> +	err = register_hotcpu_notifier(&vi->nb);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>  #endif
> -- 
> 1.8.1.2

^ permalink raw reply

* Re: [PATCH net-next] ipv4: introduce new IP_MTU_DISCOVER mode IP_PMTUDISC_INTERFACE
From: Florian Weimer @ 2013-10-29  7:36 UTC (permalink / raw)
  To: David Miller, hannes; +Cc: netdev
In-Reply-To: <20131029.000844.1092862708536984032.davem@davemloft.net>

On 10/29/2013 05:08 AM, David Miller wrote:

> I do not like this reasoning.  You have several more acceptable paths to take
> to resolve this problem:
>
> 1) "I don't trust path MTU information at all"
>
>     Just turn it off globally, end of story.  It has the same effect as your
>     new per-application mode.

We can't push this as a security update.  We could tell everyone running 
DNS servers to reconfigure their systems in this way, but I always 
consider this a bit of a cop-out.

A new knob to turn IP_PMTUDISC_DONT into something that behaves like 
IP_PMTUDISC_INTERFACE would be more conservative and easier to deploy, I 
think.

> 2) "I don't trust path MTU information unless the full socket ID is available
>      in the ICMP packets quoted headers"
>
>     Then simply implement a policy as such and submit it to me.

There are IP protocols where these bits aren't readily available and 
where we don't want the kernel (outside the Netfilter code) to be aware 
of the payload structure.  Netfilter isn't a solution because it 
requires state and doesn't work well with request-response UDP protocols 
like DNS (even before source port randomization).

You could make the path MTU dependent on the protocol (which would even 
be the correct solution from a technical point of view) and use 
validation for TCP and UDP, but that's a fairly invasive change for such 
relatively minor functionality.

-- 
Florian Weimer / Red Hat Product Security Team

^ permalink raw reply

* [PATCH net 0/3] r8152 bug fixes
From: Hayes Wang @ 2013-10-29  7:56 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

These could fix some driver issues.

Hayes Wang (3):
  r8152: fix tx/rx memory overflow
  r8152: modify the tx flow
  r8152: fix incorrect type in assignment

 drivers/net/usb/r8152.c | 100 +++++++++++++++++-------------------------------
 1 file changed, 36 insertions(+), 64 deletions(-)

-- 
1.8.3.1

^ permalink raw reply

* [PATCH net 1/3] r8152: fix tx/rx memory overflow
From: Hayes Wang @ 2013-10-29  7:56 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1383033377-1178-1-git-send-email-hayeswang@realtek.com>

The tx/rx would access the memory which is out of the desired range.
Modify the method of checking the end of the memory to avoid it.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f3fce41..5dbfe50 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -24,7 +24,7 @@
 #include <linux/ipv6.h>
 
 /* Version Information */
-#define DRIVER_VERSION "v1.01.0 (2013/08/12)"
+#define DRIVER_VERSION "v1.02.0 (2013/10/28)"
 #define DRIVER_AUTHOR "Realtek linux nic maintainers <nic_swsd@realtek.com>"
 #define DRIVER_DESC "Realtek RTL8152 Based USB 2.0 Ethernet Adapters"
 #define MODULENAME "r8152"
@@ -1136,14 +1136,14 @@ r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc, struct sk_buff *skb)
 
 static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 {
-	u32 remain;
+	int remain;
 	u8 *tx_data;
 
 	tx_data = agg->head;
 	agg->skb_num = agg->skb_len = 0;
-	remain = rx_buf_sz - sizeof(struct tx_desc);
+	remain = rx_buf_sz;
 
-	while (remain >= ETH_ZLEN) {
+	while (remain >= ETH_ZLEN + sizeof(struct tx_desc)) {
 		struct tx_desc *tx_desc;
 		struct sk_buff *skb;
 		unsigned int len;
@@ -1152,12 +1152,14 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 		if (!skb)
 			break;
 
+		remain -= sizeof(*tx_desc);
 		len = skb->len;
 		if (remain < len) {
 			skb_queue_head(&tp->tx_queue, skb);
 			break;
 		}
 
+		tx_data = tx_agg_align(tx_data);
 		tx_desc = (struct tx_desc *)tx_data;
 		tx_data += sizeof(*tx_desc);
 
@@ -1167,9 +1169,8 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 		agg->skb_len += len;
 		dev_kfree_skb_any(skb);
 
-		tx_data = tx_agg_align(tx_data + len);
-		remain = rx_buf_sz - sizeof(*tx_desc) -
-			 (u32)((void *)tx_data - agg->head);
+		tx_data += len;
+		remain = rx_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);
 	}
 
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),
@@ -1188,7 +1189,6 @@ static void rx_bottom(struct r8152 *tp)
 	list_for_each_safe(cursor, next, &tp->rx_done) {
 		struct rx_desc *rx_desc;
 		struct rx_agg *agg;
-		unsigned pkt_len;
 		int len_used = 0;
 		struct urb *urb;
 		u8 *rx_data;
@@ -1204,17 +1204,22 @@ static void rx_bottom(struct r8152 *tp)
 
 		rx_desc = agg->head;
 		rx_data = agg->head;
-		pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
-		len_used += sizeof(struct rx_desc) + pkt_len;
+		len_used += sizeof(struct rx_desc);
 
-		while (urb->actual_length >= len_used) {
+		while (urb->actual_length > len_used) {
 			struct net_device *netdev = tp->netdev;
 			struct net_device_stats *stats;
+			unsigned pkt_len;
 			struct sk_buff *skb;
 
+			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
 			if (pkt_len < ETH_ZLEN)
 				break;
 
+			len_used += pkt_len;
+			if (urb->actual_length < len_used)
+				break;
+
 			stats = rtl8152_get_stats(netdev);
 
 			pkt_len -= 4; /* CRC */
@@ -1234,9 +1239,8 @@ static void rx_bottom(struct r8152 *tp)
 
 			rx_data = rx_agg_align(rx_data + pkt_len + 4);
 			rx_desc = (struct rx_desc *)rx_data;
-			pkt_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
 			len_used = (int)(rx_data - (u8 *)agg->head);
-			len_used += sizeof(struct rx_desc) + pkt_len;
+			len_used += sizeof(struct rx_desc);
 		}
 
 submit:
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 2/3] r8152: modify the tx flow
From: Hayes Wang @ 2013-10-29  7:56 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1383033377-1178-1-git-send-email-hayeswang@realtek.com>

Let rtl8152_start_xmit() to queue packet only, and tx_bottom() would
send all of the packets. This simplify the code and make sure all the
packet would be sent by the original order.

Support stopping and waking tx queue. The maximum tx queue length
is 60.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 52 ++++++++++---------------------------------------
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 5dbfe50..1647931 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -276,6 +276,8 @@ enum rtl_register_content {
 
 #define INTR_LINK		0x0004
 
+#define TX_QLEN			60
+
 #define RTL8152_REQT_READ	0xc0
 #define RTL8152_REQT_WRITE	0x40
 #define RTL8152_REQ_GET_REGS	0x05
@@ -1173,6 +1175,9 @@ static int r8152_tx_agg_fill(struct r8152 *tp, struct tx_agg *agg)
 		remain = rx_buf_sz - (int)(tx_agg_align(tx_data) - agg->head);
 	}
 
+	if (netif_queue_stopped(tp->netdev))
+		netif_wake_queue(tp->netdev);
+
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),
 			  agg->head, (int)(tx_data - (u8 *)agg->head),
 			  (usb_complete_t)write_bulk_callback, agg);
@@ -1388,53 +1393,16 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
 					    struct net_device *netdev)
 {
 	struct r8152 *tp = netdev_priv(netdev);
-	struct net_device_stats *stats = rtl8152_get_stats(netdev);
-	unsigned long flags;
-	struct tx_agg *agg = NULL;
-	struct tx_desc *tx_desc;
-	unsigned int len;
-	u8 *tx_data;
-	int res;
 
 	skb_tx_timestamp(skb);
 
-	/* If tx_queue is not empty, it means at least one previous packt */
-	/* is waiting for sending. Don't send current one before it.      */
-	if (skb_queue_empty(&tp->tx_queue))
-		agg = r8152_get_tx_agg(tp);
-
-	if (!agg) {
-		skb_queue_tail(&tp->tx_queue, skb);
-		return NETDEV_TX_OK;
-	}
+	skb_queue_tail(&tp->tx_queue, skb);
 
-	tx_desc = (struct tx_desc *)agg->head;
-	tx_data = agg->head + sizeof(*tx_desc);
-	agg->skb_num = agg->skb_len = 0;
+	if (skb_queue_len(&tp->tx_queue) > TX_QLEN)
+		netif_stop_queue(netdev);
 
-	len = skb->len;
-	r8152_tx_csum(tp, tx_desc, skb);
-	memcpy(tx_data, skb->data, len);
-	dev_kfree_skb_any(skb);
-	agg->skb_num++;
-	agg->skb_len += len;
-	usb_fill_bulk_urb(agg->urb, tp->udev, usb_sndbulkpipe(tp->udev, 2),
-			  agg->head, len + sizeof(*tx_desc),
-			  (usb_complete_t)write_bulk_callback, agg);
-	res = usb_submit_urb(agg->urb, GFP_ATOMIC);
-	if (res) {
-		/* Can we get/handle EPIPE here? */
-		if (res == -ENODEV) {
-			netif_device_detach(tp->netdev);
-		} else {
-			netif_warn(tp, tx_err, netdev,
-				   "failed tx_urb %d\n", res);
-			stats->tx_dropped++;
-			spin_lock_irqsave(&tp->tx_lock, flags);
-			list_add_tail(&agg->list, &tp->tx_free);
-			spin_unlock_irqrestore(&tp->tx_lock, flags);
-		}
-	}
+	if (!list_empty(&tp->tx_free))
+		tasklet_schedule(&tp->tl);
 
 	return NETDEV_TX_OK;
 }
-- 
1.8.3.1

^ permalink raw reply related

* [PATCH net 3/3] r8152: fix incorrect type in assignment
From: Hayes Wang @ 2013-10-29  7:56 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang
In-Reply-To: <1383033377-1178-1-git-send-email-hayeswang@realtek.com>

Correct some declaration.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1647931..b92b7f4 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -309,22 +309,22 @@ enum rtl8152_flags {
 #define MCU_TYPE_USB			0x0000
 
 struct rx_desc {
-	u32 opts1;
+	__le32 opts1;
 #define RX_LEN_MASK			0x7fff
-	u32 opts2;
-	u32 opts3;
-	u32 opts4;
-	u32 opts5;
-	u32 opts6;
+	__le32 opts2;
+	__le32 opts3;
+	__le32 opts4;
+	__le32 opts5;
+	__le32 opts6;
 };
 
 struct tx_desc {
-	u32 opts1;
+	__le32 opts1;
 #define TX_FS			(1 << 31) /* First segment of a packet */
 #define TX_LS			(1 << 30) /* Final segment of a packet */
 #define TX_LEN_MASK		0x3ffff
 
-	u32 opts2;
+	__le32 opts2;
 #define UDP_CS			(1 << 31) /* Calculate UDP/IP checksum */
 #define TCP_CS			(1 << 30) /* Calculate TCP/IP checksum */
 #define IPV4_CS			(1 << 29) /* Calculate IPv4 checksum */
@@ -878,7 +878,7 @@ static void write_bulk_callback(struct urb *urb)
 static void intr_callback(struct urb *urb)
 {
 	struct r8152 *tp;
-	__u16 *d;
+	__le16 *d;
 	int status = urb->status;
 	int res;
 
-- 
1.8.3.1

^ permalink raw reply related

* Re: [PATCH] bgmac: don't update slot on skb alloc/dma mapping error
From: Nathan Hintz @ 2013-10-29  8:22 UTC (permalink / raw)
  To: Rafał Miłecki; +Cc: Network Development
In-Reply-To: <CACna6rzxt2QMtLfD84pw7EtYqg1Qva4ah83hVQOKpeKy72bd5w@mail.gmail.com>

On Tue, 29 Oct 2013 07:52:58 +0100
Rafał Miłecki <zajec5@gmail.com> wrote:

> 2013/10/29 Nathan Hintz <nlhintz@hotmail.com>:
> > Don't update the slot in "bgmac_dma_rx_skb_for_slot" unless both the
> > skb alloc and dma mapping are successful; and free the newly allocated
> > skb if a dma mapping error occurs.  This will prevent an skb leak upon
> > returning when an error occurs.
> 
> In case of bgmac_dma_rx_skb_for_slot failure we're giving up anyway
> (and freeing everything), but with your patch code is simpler to
> understand, so I'm OK with that.
> 
> Acked-by: Rafał Miłecki <zajec5@gmail.com>
> 

I might be misunderstanding; but it in the case of failure, it appeared to me
that the currently received packet was dropped and the old skb would continue
to be assigned to the slot and would be used to receive future packets (this
would continue until bgmac_dma_rx_skb_for_slot was successful).

-- 
Nathan

^ permalink raw reply

* Re: [PATCH] x86: Run checksumming in parallel accross multiple alu's
From: Ingo Molnar @ 2013-10-29  8:25 UTC (permalink / raw)
  To: Neil Horman
  Cc: Eric Dumazet, linux-kernel, sebastien.dugue, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86, netdev
In-Reply-To: <20131028182913.GD31048@hmsreliant.think-freely.org>


* Neil Horman <nhorman@tuxdriver.com> wrote:

> Heres my data for running the same test with taskset restricting 
> execution to only cpu0.  I'm not quite sure whats going on here, 
> but doing so resulted in a 10x slowdown of the runtime of each 
> iteration which I can't explain.  As before however, both the 
> parallel alu run and the prefetch run resulted in speedups, but 
> the two together were not in any way addative.  I'm going to keep 
> playing with the prefetch stride, unless you have an alternate 
> theory.

Could you please cite the exact command-line you used for running 
the test?

Thanks,

	Ingo

^ permalink raw reply

* [PATCH v2] can: c_can: Speed up rx_poll function
From: Markus Pargmann @ 2013-10-29  8:27 UTC (permalink / raw)
  To: Marc Kleine-Budde, Wolfgang Grandegger
  Cc: Joe Perches, linux-can, netdev, linux-kernel, kernel,
	Markus Pargmann

This patch speeds up the rx_poll function by reducing the number of
register reads.

Replace the 32bit register read by a 16bit register read. Currently
the 32bit register read is implemented by using 2 16bit reads. This is
inefficient as we only use the lower 16bit in rx_poll.

The for loop reads the pending interrupts in every iteration. This
leads up to 16 reads of pending interrupts. The patch introduces a new
outer loop to read the pending interrupts as long as 'quota' is above 0.
This reduces the total number of reads.

The third change is to replace the for-loop by a find_next_bit loop.

Tested on AM335x. I removed all 'static' and 'inline' from c_can.c to
see the timings for all functions. I used the function tracer with
trace_stats.

125kbit:
  Function                               Hit    Time            Avg             s^2
  --------                               ---    ----            ---             ---
  c_can_do_rx_poll                     63960    10168178 us     158.977 us      1493056 us
With patch:
  c_can_do_rx_poll                     63939    4268457 us     66.758 us       818790.9 us

1Mbit:
  Function                               Hit    Time            Avg             s^2
  --------                               ---    ----            ---             ---
  c_can_do_rx_poll                     69489    30049498 us     432.435 us      9271851 us
With patch:
  c_can_do_rx_poll                    103034    24220362 us     235.071 us      6016656 us

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---

Notes:
    Changes in v2:
     - Small changes, find_next_bit -> ffs and other

 drivers/net/can/c_can/c_can.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index a668cd4..428681e 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -798,17 +798,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	u32 num_rx_pkts = 0;
 	unsigned int msg_obj, msg_ctrl_save;
 	struct c_can_priv *priv = netdev_priv(dev);
-	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
+	u16 val;
+
+	/*
+	 * It is faster to read only one 16bit register. This is only possible
+	 * for a maximum number of 16 objects.
+	 */
+	BUILD_BUG_ON_MSG(C_CAN_MSG_OBJ_RX_LAST > 16,
+			"Implementation does not support more message objects than 16");
+
+	while (quota > 0 && (val = priv->read_reg(priv, C_CAN_INTPND1_REG))) {
+		while ((msg_obj = ffs(val)) && quota > 0) {
+			val &= ~BIT(msg_obj - 1);
 
-	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
-			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
-			val = c_can_read_reg32(priv, C_CAN_INTPND1_REG),
-			msg_obj++) {
-		/*
-		 * as interrupt pending register's bit n-1 corresponds to
-		 * message object n, we need to handle the same properly.
-		 */
-		if (val & (1 << (msg_obj - 1))) {
 			c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL &
 					~IF_COMM_TXRQST);
 			msg_ctrl_save = priv->read_reg(priv,
-- 
1.8.4.rc3


^ permalink raw reply related


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