netdev - Re: [PATCH 11/11] usbnet: handle link change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 11 Apr 2013 22:52:29 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	"David S. Miller" <davem@...emloft.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	Oliver Neukum <oneukum@...e.de>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, Ming Lei <ming.lei@...onical.com>
Subject: Re: [PATCH 11/11] usbnet: handle link change

On Thu, Apr 11, 2013 at 10:40 PM, Ming Lei <ming.lei@...onical.com> wrote:
> The link change is detected via the interrupt pipe, and bulk
> pipes are responsible for transfering packets, so it is reasonable
> to stop bulk transfer after link is reported as off.
>
> Two adavantages may be obtained with stopping bulk transfer
> after link becomes off:
>
> - USB bus bandwidth is saved(USB bus is shared bus except for
> USB3.0), for example, lots of 'IN' token packets and 'NYET'
> handshake packets is transfered on 2.0 bus.
>
> - probabaly power might be saved for usb host controller since
> cancelling bulk transfer may disable the asynchronous schedule of
> host controller.
>
> With this patch, when link becomes off, about ~10% performance
> boost can be found on bulk transfer of anther usb device which
> is attached to same bus with the usbnet device, see below
> test on next-20130410:
>
> - read from usb mass storage(Sandisk Extreme USB 3.0) on pandaboard
> with below command after unplugging ethernet cable:
>
>         dd if=/dev/sda iflag=direct of=/dev/null bs=1M count=500

Sorry, the above should be:

        dd if=/dev/sda iflag=direct of=/dev/null bs=1M count=800

>
> - without the patch
> 1, 838860800 bytes (839 MB) copied, 36.2216 s, 23.2 MB/s
> 2, 838860800 bytes (839 MB) copied, 35.8368 s, 23.4 MB/s
> 3, 838860800 bytes (839 MB) copied, 35.823 s, 23.4 MB/s
> 4, 838860800 bytes (839 MB) copied, 35.937 s, 23.3 MB/s
> 5, 838860800 bytes (839 MB) copied, 35.7365 s, 23.5 MB/s
> average: 23.6MB/s
>
> - with the patch
> 1, 838860800 bytes (839 MB) copied, 32.3817 s, 25.9 MB/s
> 2, 838860800 bytes (839 MB) copied, 31.7389 s, 26.4 MB/s
> 3, 838860800 bytes (839 MB) copied, 32.438 s, 25.9 MB/s
> 4, 838860800 bytes (839 MB) copied, 32.5492 s, 25.8 MB/s
> 5, 838860800 bytes (839 MB) copied, 31.6178 s, 26.5 MB/s
> average: 26.1MB/s
>
> Signed-off-by: Ming Lei <ming.lei@...onical.com>
> ---
>  drivers/net/usb/usbnet.c   |   30 ++++++++++++++++++++++++++++++
>  include/linux/usb/usbnet.h |    1 +
>  2 files changed, 31 insertions(+)
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 34e4252..1e5a9b7 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -938,6 +938,27 @@ static const struct ethtool_ops usbnet_ethtool_ops = {
>
>  /*-------------------------------------------------------------------------*/
>
> +static void __handle_link_change(struct usbnet *dev)
> +{
> +       if (!test_bit(EVENT_DEV_OPEN, &dev->flags))
> +               return;
> +
> +       if (!netif_carrier_ok(dev->net)) {
> +               /* kill URBs for reading packets to save bus bandwidth */
> +               unlink_urbs(dev, &dev->rxq);
> +
> +               /*
> +                * tx_timeout will unlink URBs for sending packets and
> +                * tx queue is stopped by netcore after link becomes off
> +                */
> +       } else {
> +               /* submitting URBs for reading packets */
> +               tasklet_schedule(&dev->bh);
> +       }
> +
> +       clear_bit(EVENT_LINK_CHANGE, &dev->flags);
> +}
> +
>  /* work that cannot be done in interrupt context uses keventd.
>   *
>   * NOTE:  with 2.5 we could do more of this using completion callbacks,
> @@ -1035,8 +1056,14 @@ skip_reset:
>                 } else {
>                         usb_autopm_put_interface(dev->intf);
>                 }
> +
> +               /* handle link change from link resetting */
> +               __handle_link_change(dev);
>         }
>
> +       if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
> +               __handle_link_change(dev);
> +
>         if (dev->flags)
>                 netdev_dbg(dev->net, "kevent done, flags = 0x%lx\n", dev->flags);
>  }
> @@ -1286,6 +1313,7 @@ static void usbnet_bh (unsigned long param)
>         // or are we maybe short a few urbs?
>         } else if (netif_running (dev->net) &&
>                    netif_device_present (dev->net) &&
> +                  netif_carrier_ok(dev->net) &&
>                    !timer_pending (&dev->delay) &&
>                    !test_bit (EVENT_RX_HALT, &dev->flags)) {
>                 int     temp = dev->rxq.qlen;
> @@ -1663,6 +1691,8 @@ void usbnet_link_change(struct usbnet *dev, bool link, bool need_reset)
>
>         if (need_reset && link)
>                 usbnet_defer_kevent(dev, EVENT_LINK_RESET);
> +       else
> +               usbnet_defer_kevent(dev, EVENT_LINK_CHANGE);
>  }
>  EXPORT_SYMBOL(usbnet_link_change);
>
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index eb021b8..da46327 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -72,6 +72,7 @@ struct usbnet {
>  #              define EVENT_DEVICE_REPORT_IDLE 8
>  #              define EVENT_NO_RUNTIME_PM      9
>  #              define EVENT_RX_KILL    10
> +#              define EVENT_LINK_CHANGE        11
>  };
>
>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html