* Re: [patch net-next RFC 10/12] openvswitch: add support for datapath hardware offload
From: Simon Horman @ 2014-09-05 4:08 UTC (permalink / raw)
To: Scott Feldman
Cc: ryazanov.s.a-Re5JQEeQqe8AvxtiuMwx3w,
jasowang-H+wXaHxf7aLQT0dZR+AlfA,
john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w,
Neil.Jerram-QnUH15yq9NYqDJ6do+/SaQ,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, andy-QlMahl40kYEqcZcGjlUOXw,
dev-yBygre7rU0TnMu66kgdUjQ, nbd-p3rKhJxN3npAfugRpC6u6w,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, ronye-VPRAkNaXOzVWk0Htik3J/w,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, ben-/+tVBieCtBitmTQ+vhA3Yw,
buytenh-OLH4Qvv75CYX/NnBR394Jw, Jiri Pirko,
roopa-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR,
jhs-jkUAjuhPggJWk0Htik3J/w, aviadr-VPRAkNaXOzVWk0Htik3J/w,
nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w,
vyasevic-H+wXaHxf7aLQT0dZR+AlfA, nhorman-2XuSBdqkA4R54TAoqtyWWQ,
netdev-u79uwXL29TY76Z2rM5mHXA,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <F4498A89-C1D6-4C5A-A6F0-942015D36B77-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
On Thu, Sep 04, 2014 at 09:30:45AM -0700, Scott Feldman wrote:
>
> On Sep 4, 2014, at 2:04 AM, Simon Horman <simon.horman@netronome.com> wrote:
>
> >
> >
> > [snip]
> >
> > In relation to ports and datapaths it seems to me that the API that
> > has been developed accommodates a model where a port may belong to a
> > switch device; that this topology is fixed before any API calls are made
> > and that all all ports belonging to the same switch belong to the same
> > datapath.
> >
> > This makes sense in the case of hardware that looks a lot like a switch.
> > But I think that other scenarios are possible. For example hardware that
> > is able to handle the same abstractions handled by the datapath: datapaths
> > may be created or destroyed; vports may be added and removed from datapaths.
> >
> > So one might have a piece of hardware that is configured with more than one
> > datapath configured and its different ports belong to it might be
> > associated with different data paths.
>
> I’ve tested multiple datapaths on one switch hardware with the current patch set and it works fine, without the need to push down any datapath id in the API. It works because a switch port can’t belong to more than one datapath. Datapaths can be created/destroyed and ports added/removed from datapaths dynamically and the right sw_flows are added/removed to program HW.
And the flows added to a switch always match the in port? Thus
so a given flow is only ever for one in-port and thus one datapath?
> > Or we might have hardware that is able to offload a tunnel vport.
I think tunnel vports is still an unsolved part of the larger puzzle.
> > In short I am thinking in terms of API callbacks to manipulate datapaths
> > and vports. Although I have not thought about it in detail I believe
> > that the current model you have implemented using such a scheme because
> > the scheme I am suggesting maps to that of the datapath and you have
> > implemented your model there.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Re: [patch net-next 10/13] openvswitch: add support for datapath hardware offload
From: Simon Horman @ 2014-09-05 3:59 UTC (permalink / raw)
To: Jiri Pirko
Cc: ryazanov.s.a-Re5JQEeQqe8AvxtiuMwx3w, John Fastabend,
jasowang-H+wXaHxf7aLQT0dZR+AlfA,
john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w,
Neil.Jerram-QnUH15yq9NYqDJ6do+/SaQ,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, andy-QlMahl40kYEqcZcGjlUOXw,
dev-yBygre7rU0TnMu66kgdUjQ, nbd-p3rKhJxN3npAfugRpC6u6w,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, ronye-VPRAkNaXOzVWk0Htik3J/w,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, ben-/+tVBieCtBitmTQ+vhA3Yw,
buytenh-OLH4Qvv75CYX/NnBR394Jw,
roopa-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR,
jhs-jkUAjuhPggJWk0Htik3J/w, aviadr-VPRAkNaXOzVWk0Htik3J/w,
nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w,
vyasevic-H+wXaHxf7aLQT0dZR+AlfA, nhorman-2XuSBdqkA4R54TAoqtyWWQ,
netdev-u79uwXL29TY76Z2rM5mHXA,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <20140904124837.GI1867-6KJVSR23iU5sFDB2n11ItA@public.gmane.org>
On Thu, Sep 04, 2014 at 02:48:37PM +0200, Jiri Pirko wrote:
> Wed, Sep 03, 2014 at 06:37:08PM CEST, john.fastabend-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> >On 09/03/2014 02:24 AM, Jiri Pirko wrote:
> >>Benefit from the possibility to work with flows in switch devices and
> >>use the swdev api to offload flow datapath.
> >>
> >>Signed-off-by: Jiri Pirko <jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
> >>---
> >> net/openvswitch/Makefile | 3 +-
> >> net/openvswitch/datapath.c | 33 ++++++
> >> net/openvswitch/datapath.h | 3 +
> >> net/openvswitch/flow_table.c | 1 +
> >> net/openvswitch/hw_offload.c | 245 +++++++++++++++++++++++++++++++++++++++++
> >> net/openvswitch/hw_offload.h | 22 ++++
> >> net/openvswitch/vport-netdev.c | 3 +
> >> net/openvswitch/vport.h | 2 +
> >> 8 files changed, 311 insertions(+), 1 deletion(-)
> >> create mode 100644 net/openvswitch/hw_offload.c
> >> create mode 100644 net/openvswitch/hw_offload.h
> >>
> >>diff --git a/net/openvswitch/Makefile b/net/openvswitch/Makefile
> >>index 3591cb5..5152437 100644
> >>--- a/net/openvswitch/Makefile
> >>+++ b/net/openvswitch/Makefile
> >>@@ -13,7 +13,8 @@ openvswitch-y := \
> >> flow_table.o \
> >> vport.o \
> >> vport-internal_dev.o \
> >>- vport-netdev.o
> >>+ vport-netdev.o \
> >>+ hw_offload.o
> >>
> >> ifneq ($(CONFIG_OPENVSWITCH_VXLAN),)
> >> openvswitch-y += vport-vxlan.o
> >>diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> >>index 75bb07f..3e43e1d 100644
> >>--- a/net/openvswitch/datapath.c
> >>+++ b/net/openvswitch/datapath.c
> >>@@ -57,6 +57,7 @@
> >> #include "flow_netlink.h"
> >> #include "vport-internal_dev.h"
> >> #include "vport-netdev.h"
> >>+#include "hw_offload.h"
> >>
> >> int ovs_net_id __read_mostly;
> >>
> >>@@ -864,6 +865,9 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> >> acts = NULL;
> >> goto err_unlock_ovs;
> >> }
> >>+ error = ovs_hw_flow_insert(dp, new_flow);
> >>+ if (error)
> >>+ pr_warn("failed to insert flow into hw\n");
> >
> >This is really close to silently failing. I think we need to
> >hard fail here somehow and push it back to userspace as part of
> >the reply and ovs_notify.
>
> Yes, I agree. My plan was to handle this in ovs hw/sw/both netlink attr
> implementation.
FWIW I agree that handling it in that way makes sense.
In particular I think that "both", where the datapath is allowed
to fall back to software is a useful mode to have (its the current
implementation, right?). But that it is also good to allow user-space
more control.
> >Otherwise I don't know how to manage the hardware correctly. Consider
> >the hardware table is full. In this case user space will continue to
> >add rules and they will be silently discarded. Similarly if user space
> >adds a flow/action that can not be supported by the hardware it will
> >be silently ignored.
> >
> >Even if we do careful accounting on resources in user space we could
> >still get an ENOMEM error from sw_flow_action_create.
> >
> >Same comment for the other hw commands flush/remove.
> >
> >> if (unlikely(reply)) {
> >> error = ovs_flow_cmd_fill_info(new_flow,
> >>@@ -896,10 +900,18 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
> >> goto err_unlock_ovs;
> >> }
> >> }
> >
> >
> >[...]
> >
> >
> >Thanks,
> >John
> >
> >--
> >John Fastabend Intel Corporation
> _______________________________________________
> dev mailing list
> dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
> http://openvswitch.org/mailman/listinfo/dev
>
^ permalink raw reply
* Re: Regression: TCP connections fail over wireless: bad cksum?
From: Tom Herbert @ 2014-09-05 3:41 UTC (permalink / raw)
To: Ted Percival; +Cc: Eric Dumazet, Linux Netdev List
In-Reply-To: <5408F3FB.2010107@tedp.id.au>
On Thu, Sep 4, 2014 at 4:21 PM, Ted Percival <ted@tedp.id.au> wrote:
> On 09/04/2014 04:50 PM, Eric Dumazet wrote:
>> On Thu, 2014-09-04 at 14:11 -0600, Ted Percival wrote:
>>> Yesterday's linux-next build introduced a problem with wireless
>>> networking on my machine. ie. next-20140901 worked fine but
>>> next-20140902 does not seem able to sustain a TCP connection over
>>> wireless. Wired networking works fine. I am using the brcmsmac driver
>>> and the hardware is "Broadcom Corporation BCM4313 802.11b/g/n Wireless
>>> LAN Controller (rev 01)".
>>>
>>> Pings, even large pings (ping -s 16000) work fine but TCP connections hang.
>>>
>>> I looked through the changes between the bad & good commits from the net
>>> & net-next trees and I wonder if some of the changes to checksumming
>>> have surfaced a problem with this driver. When I look at a tcpdump, it
>>> indicates that all the checksums are wrong (although I don't know if
>>> that is just due to hardware offload).
>>>
>>> Here is a short trace of the hung connection attempt of
>>> curl http://lwn.net/
>>>
>>> $ sudo tcpdump -vvn -i wlan0 port 80
>>> tcpdump: listening on wlan0, link-type EN10MB (Ethernet), capture size
>>> 65535 bytes
>>> 13:20:15.120770 IP (tos 0x0, ttl 64, id 22734, offset 0, flags [DF],
>>> proto TCP (6), length 60)
>>> 10.5.51.93.38035 > 72.51.34.34.80: Flags [S], cksum 0xa7e5
>>> (incorrect -> 0x4fac), seq 2861986597, win 29200, options [mss
>>> 1460,sackOK,TS val 142315 ecr 0,nop,wscale 7], length 0
>>> 13:20:16.121755 IP (tos 0x0, ttl 64, id 22735, offset 0, flags [DF],
>>> proto TCP (6), length 60)
>>> 10.5.51.93.38035 > 72.51.34.34.80: Flags [S], cksum 0xa7e5
>>> (incorrect -> 0x4bc3), seq 2861986597, win 29200, options [mss
>>> 1460,sackOK,TS val 143316 ecr 0,nop,wscale 7], length 0
>>> 13:20:18.125748 IP (tos 0x0, ttl 64, id 22736, offset 0, flags [DF],
>>> proto TCP (6), length 60)
>>> 10.5.51.93.38035 > 72.51.34.34.80: Flags [S], cksum 0xa7e5
>>> (incorrect -> 0x43ef), seq 2861986597, win 29200, options [mss
>>> 1460,sackOK,TS val 145320 ecr 0,nop,wscale 7], length 0
>>> 13:20:22.133743 IP (tos 0x0, ttl 64, id 22737, offset 0, flags [DF],
>>> proto TCP (6), length 60)
>>> 10.5.51.93.38035 > 72.51.34.34.80: Flags [S], cksum 0xa7e5
>>> (incorrect -> 0x3447), seq 2861986597, win 29200, options [mss
>>> 1460,sackOK,TS val 149328 ecr 0,nop,wscale 7], length 0
>>> 13:20:30.149754 IP (tos 0x0, ttl 64, id 22738, offset 0, flags [DF],
>>> proto TCP (6), length 60)
>>> 10.5.51.93.38035 > 72.51.34.34.80: Flags [S], cksum 0xa7e5
>>> (incorrect -> 0x14f7), seq 2861986597, win 29200, options [mss
>>> 1460,sackOK,TS val 157344 ecr 0,nop,wscale 7], length 0
>>>
>>>
>>> I don't see anything that looks related in dmesg. The only brcmsmac
>>> messages I see are:
>>>
>>> [ 567.122218] brcmsmac bcma0:0: brcmsmac: brcms_ops_bss_info_changed:
>>> associated
>>> [ 567.122226] brcmsmac bcma0:0: brcms_ops_bss_info_changed: arp
>>> filtering: 1 addresses (implement)
>>> [ 567.122231] brcmsmac bcma0:0: brcms_ops_bss_info_changed: qos
>>> enabled: true (implement)
>>> [ 567.192283] brcmsmac bcma0:0: brcms_ops_bss_info_changed: qos
>>> enabled: true (implement)
>>>
>>> I am writing to linux-netdev rather than linux-wireless because
>>> according to Next/SHA1s the wireless & wireless-next trees were not
>>> updated between next-20140901 and next-20140902, but the net & net-next
>>> trees were updated, so maybe the regression came from there. (I haven't
>>> tested next-20140903 because it won't boot for unrelated reasons.)
>>>
>>> Let me know if I should just file this in Bugzilla or what information I
>>> can provide to help track this down, if it hasn't already been identified.
>>>
>>> The here are the good (-) and bad (+) trees from Next/SHA1s at the
>>> next-* tags mentioned earlier that I built from.
>>>
>>> -net 38ab1fa981d543e1b00f4ffbce4ddb480cd2effe
>>> +net cc25f0cbe4409d6a573b1f3bf7020d5b04076ee9
>>>
>>> -net-next dace1b54726bffe1c009f7661e3cee6b762f30c8
>>> +net-next 364a9e93243d1785f310c0964af0e24bf1adac03
>>>
>>
>> Could you post
>>
>> ethtool -k wlan0
>>
>> And try
>>
>> ethtool -K wlan tx off
>
> # ethtool -k wlan0
> Features for wlan0:
> rx-checksumming: off [fixed]
> tx-checksumming: off
> tx-checksum-ipv4: off [fixed]
> tx-checksum-ip-generic: off [fixed]
> tx-checksum-ipv6: off [fixed]
> tx-checksum-fcoe-crc: off [fixed]
> tx-checksum-sctp: off [fixed]
> scatter-gather: off
> tx-scatter-gather: off [fixed]
> tx-scatter-gather-fraglist: off [fixed]
> tcp-segmentation-offload: off
> tx-tcp-segmentation: off [fixed]
> tx-tcp-ecn-segmentation: off [fixed]
> tx-tcp6-segmentation: off [fixed]
> udp-fragmentation-offload: off [fixed]
> generic-segmentation-offload: off [requested on]
> generic-receive-offload: on
> large-receive-offload: off [fixed]
> rx-vlan-offload: off [fixed]
> tx-vlan-offload: off [fixed]
> ntuple-filters: off [fixed]
> receive-hashing: off [fixed]
> highdma: off [fixed]
> rx-vlan-filter: off [fixed]
> vlan-challenged: off [fixed]
> tx-lockless: off [fixed]
> netns-local: on [fixed]
> tx-gso-robust: off [fixed]
> tx-fcoe-segmentation: off [fixed]
> tx-gre-segmentation: off [fixed]
> tx-ipip-segmentation: off [fixed]
> tx-sit-segmentation: off [fixed]
> tx-udp_tnl-segmentation: off [fixed]
> tx-mpls-segmentation: off [fixed]
> fcoe-mtu: off [fixed]
> tx-nocache-copy: off
> loopback: off [fixed]
> rx-fcs: off [fixed]
> rx-all: off [fixed]
> tx-vlan-stag-hw-insert: off [fixed]
> rx-vlan-stag-hw-parse: off [fixed]
> rx-vlan-stag-filter: off [fixed]
> l2-fwd-offload: off [fixed]
> busy-poll: off [fixed]
>
> # ethtool -K wlan0 tx off
> Cannot change tx-checksumming
>
> I will try to isolate the commit that caused the regression.
Please provide 'netstat -s' to see if bad checksums are being
reported. Also, try disabling GRO to see the effect.
Thanks,
Tom
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/1] add selftest for virtio-net v1.0
From: Jason Wang @ 2014-09-05 3:31 UTC (permalink / raw)
To: Hengjinxiao, virtualization, netdev, linux-kernel, linux-api; +Cc: famz, mst
In-Reply-To: <1409881866-14780-1-git-send-email-hjxiaohust@gmail.com>
On 09/05/2014 09:51 AM, Hengjinxiao wrote:
> Selftest is an important part of network driver, this patch adds selftest for
> virtio-net, including loopback test, negotiate test and reset test. Loopback
> test checks whether virtio-net can send and receive packets normally. Negotiate test
> executes feature negotiation between virtio-net driver in Guest OS and virtio-net
> device in Host OS. Reset test resets virtio-net.
> Following last patch, this version has deleted some useless codes and fixed bugs
> as you suggest.
> Any corrections are welcome.
>
> Signed-off-by: Hengjinxiao <hjxiaohust@gmail.com>
Some of the lines were indented correctly, some others exceeded 80
characters per line. Please see Documentation/SubmittingPatches for more
guide lines. More important, some of the comments in V1 were still not
addressed.
I suggest split this patch into series:
- patch that introduces virtio core helpers
- patch that introduces new virtio-net helpers
- patch that implements a skeleton of the selftest
- patch that implements loopback test
- patch that implements feature negotation test
Thanks
>
> ---
> drivers/net/virtio_net.c | 241 ++++++++++++++++++++++++++++++++++++++--
> drivers/virtio/virtio.c | 20 +++-
> include/linux/virtio.h | 2 +
> include/uapi/linux/virtio_net.h | 9 ++
> 4 files changed, 256 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 59caa06..22d8228 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -28,6 +28,7 @@
> #include <linux/cpu.h>
> #include <linux/average.h>
> #include <net/busy_poll.h>
> +#include <linux/pci.h>
Why pci.h is needed?
>
> static int napi_weight = NAPI_POLL_WEIGHT;
> module_param(napi_weight, int, 0444);
> @@ -51,6 +52,17 @@ module_param(gso, bool, 0444);
> #define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, 256)
>
> #define VIRTNET_DRIVER_VERSION "1.0.0"
> +#define __VIRTNET_TESTING 0
> +
> +static const struct {
> + const char string[ETH_GSTRING_LEN];
> +} virtnet_gstrings_test[] = {
> + { "loopback test (offline)" },
> + { "negotiate test (offline)" },
> + { "reset test (offline)" },
> +};
> +
> +#define VIRTNET_NUM_TEST ARRAY_SIZE(virtnet_gstrings_test)
>
> struct virtnet_stats {
> struct u64_stats_sync tx_syncp;
> @@ -104,6 +116,8 @@ struct virtnet_info {
> struct send_queue *sq;
> struct receive_queue *rq;
> unsigned int status;
> + unsigned long flags;
> + atomic_t lb_count;
>
> /* Max # of queue pairs supported by the device */
> u16 max_queue_pairs;
> @@ -436,6 +450,19 @@ err_buf:
> return NULL;
> }
>
> +void virtnet_check_lb_frame(struct virtnet_info *vi,
> + struct sk_buff *skb)
> +{
> + unsigned int frame_size = skb->len;
> +
> + if (*(skb->data + 3) == 0xFF) {
> + if ((*(skb->data + frame_size / 2 + 10) == 0xBE) &&
> + (*(skb->data + frame_size / 2 + 12) == 0xAF)) {
> + atomic_dec(&vi->lb_count);
> + }
> + }
> +}
> +
> static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> {
> struct virtnet_info *vi = rq->vq->vdev->priv;
> @@ -485,7 +512,12 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> } else if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) {
> skb->ip_summed = CHECKSUM_UNNECESSARY;
> }
> -
> + /* loopback self test for ethtool */
> + if (test_bit(__VIRTNET_TESTING, &vi->flags)) {
> + virtnet_check_lb_frame(vi, skb);
> + dev_kfree_skb_any(skb);
> + return;
> + }
Again, we really need a selftest specific handler and don't put anything
in the ordinary fast path.
> skb->protocol = eth_type_trans(skb, dev);
> pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
> ntohs(skb->protocol), skb->len, skb->pkt_type);
> @@ -813,6 +845,9 @@ static int virtnet_open(struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> int i;
> + /* disallow open during test */
> + if (test_bit(__VIRTNET_TESTING, &vi->flags))
> + return -EBUSY;
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> if (i < vi->curr_queue_pairs)
> @@ -1363,12 +1398,166 @@ static void virtnet_get_channels(struct net_device *dev,
> channels->other_count = 0;
> }
>
> +static int virtnet_reset(struct virtnet_info *vi, u64 *data);
> +
> +static void virtnet_create_lb_frame(struct sk_buff *skb,
> + unsigned int frame_size)
> +{
> + memset(skb->data, 0xFF, frame_size);
> + frame_size &= ~1;
> + memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
> + memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
> + memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
> +}
> +
> +static int virtnet_start_loopback(struct virtnet_info *vi)
> +{
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_LOOPBACK,
> + VIRTIO_NET_CTRL_LOOPBACK_SET, NULL, NULL)) {
> + dev_warn(&vi->dev->dev, "Failed to set loopback.\n");
> + return -EINVAL;
> + }
> + for (i = 0; i < vi->curr_queue_pairs; i++)
> + napi_disable(&vi->rq[i].napi);
> + return 0;
> +}
> +
> +static int virtnet_run_loopback_test(struct virtnet_info *vi)
> +{
> + int i;
> + struct sk_buff *skb;
> + unsigned int size = GOOD_COPY_LEN;
> +
> + for (i = 0; i < 100; i++) {
> + skb = netdev_alloc_skb(vi->dev, size);
> + if (!skb)
> + return -ENOMEM;
> +
> + skb->queue_mapping = 0;
> + skb_put(skb, size);
> + virtnet_create_lb_frame(skb, size);
> + start_xmit(skb, vi->dev);
> + atomic_inc(&vi->lb_count);
> + }
> + free_old_xmit_skbs(&vi->sq[skb->queue_mapping]);
You could not assume the packets were all sent at this time. It may be
delayed by several reasons e.g host load or zerocopy enabled.
> + /* Give queue time to settle before testing results. */
> + msleep(20);
> + for (i = 0; i < vi->curr_queue_pairs; i++) {
> + void *buf;
> + unsigned int len, received = 0;
> +
> + while ((received < 100) &&
> + (buf = virtqueue_get_buf(vi->rq[i].vq, &len)) != NULL) {
Please use a test specific rx handler for this. And the code here
duplicates the code of virtnet_receive(), you can just pass 100 as
budget to that function.
> + receive_buf(&vi->rq[i], buf, len);
> + --vi->rq[i].num;
> + received++;
> + }
> + if ((vi->rq[i].vq)->num_free <
> + virtqueue_get_vring_size(vi->rq[i].vq) / 2)
> + if (!try_fill_recv(&vi->rq[i], GFP_ATOMIC))
> + schedule_delayed_work(&vi->refill, 0);
You disable NAPI but schedule the refill work, it won't be work until
you enable the NAPI. What's more serious is that if there's a refill
work who want to run at the same time of the beginning of the loopback
test, the virtqueue will never get refilled and the test may fail.
Mixing selftest with real workload brings a lot of complexity. Lots of
thing get easier when you using a dedicated vq handler for selftest:
- Don't bother fast path.
- The rx virtqueue will be empty and completely refilled, so if you're
testing 100 packets which is smaller than 256, no need to care about the
refill, (if you detect a fill need, it was probably a bug)
- You can use polling for rx by just disabling the rx notifiy.
- The tx virtqueue will be empty and you can just detach all skbs at the
end of test.
> + }
> + return atomic_read(&vi->lb_count) ? -EIO : 0;
> +}
> +
> +static int virtnet_stop_loopback(struct virtnet_info *vi)
> +{
> + int i;
> +
> + for (i = 0; i < vi->curr_queue_pairs; i++)
> + virtnet_napi_enable(&vi->rq[i]);
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_LOOPBACK,
> + VIRTIO_NET_CTRL_LOOPBACK_UNSET, NULL, NULL)) {
> + dev_warn(&vi->dev->dev, "Failed to unset loopback.\n");
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int virtnet_loopback_test(struct virtnet_info *vi, u64 *data)
> +{
> + *data = virtnet_start_loopback(vi);
> + if (*data)
> + goto out;
> + *data = virtnet_run_loopback_test(vi);
> + if (*data)
> + goto out;
Again, you may also need to stop the loopback test in this case?
> + *data = virtnet_stop_loopback(vi);
> +out:
> + return *data;
> +}
> +
> +static int virtnet_feature_neg_test(struct virtnet_info *vi, u64 *data)
> +{
> + struct virtio_device *dev = vi->vdev;
> + u8 status;
> +
> + status = dev->config->get_status(dev);
> + if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> + u8 test_status = status & ~VIRTIO_CONFIG_S_DRIVER_OK;
> +
> + dev->config->set_status(dev, test_status);
> + }
> + *data = virtio_feature_negotiate(dev);
> + dev->config->set_status(dev, status);
> + return *data;
> +}
> +
> +static int virtnet_get_sset_count(struct net_device *netdev, int sset)
> +{
> + switch (sset) {
> + case ETH_SS_TEST:
> + return VIRTNET_NUM_TEST;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
> +{
> + switch (stringset) {
> + case ETH_SS_TEST:
> + memcpy(buf, &virtnet_gstrings_test,
> + sizeof(virtnet_gstrings_test));
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static void virtnet_self_test(struct net_device *netdev,
> + struct ethtool_test *eth_test, u64 *data)
> +{
> + struct virtnet_info *vi = netdev_priv(netdev);
> +
> + memset(data, 0, sizeof(u64) * VIRTNET_NUM_TEST);
> + if (netif_running(netdev)) {
> + set_bit(__VIRTNET_TESTING, &vi->flags);
> + if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
> + if (virtnet_loopback_test(vi, &data[0]))
> + eth_test->flags |= ETH_TEST_FL_FAILED;
> + if (virtnet_feature_neg_test(vi, &data[1]))
> + eth_test->flags |= ETH_TEST_FL_FAILED;
> + if (virtnet_reset(vi, &data[2]))
> + eth_test->flags |= ETH_TEST_FL_FAILED;
virtnet_reset() sounds like a generic helper, better not use a ehttool
specific parameter.
> + }
> + clear_bit(__VIRTNET_TESTING, &vi->flags);
> + } else {
> + dev_warn(&vi->dev->dev,
> + "%s is down, Loopback test will fail.\n", netdev->name);
> + eth_test->flags |= ETH_TEST_FL_FAILED;
> + }
> +}
> +
> static const struct ethtool_ops virtnet_ethtool_ops = {
> .get_drvinfo = virtnet_get_drvinfo,
> .get_link = ethtool_op_get_link,
> .get_ringparam = virtnet_get_ringparam,
> .set_channels = virtnet_set_channels,
> .get_channels = virtnet_get_channels,
> + .self_test = virtnet_self_test,
> + .get_strings = virtnet_get_strings,
> + .get_sset_count = virtnet_get_sset_count,
> };
>
> #define MIN_MTU 68
> @@ -1890,14 +2079,10 @@ static void virtnet_remove(struct virtio_device *vdev)
> free_netdev(vi->dev);
> }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int virtnet_freeze(struct virtio_device *vdev)
> +static void virtnet_stop(struct virtnet_info *vi)
Please split those introduction of new helpers into separate patches.
> {
> - 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;
> @@ -1906,24 +2091,20 @@ static int virtnet_freeze(struct virtio_device *vdev)
> netif_device_detach(vi->dev);
> cancel_delayed_work_sync(&vi->refill);
>
> - if (netif_running(vi->dev)) {
> + if (netif_running(vi->dev))
> for (i = 0; i < vi->max_queue_pairs; i++) {
> napi_disable(&vi->rq[i].napi);
> napi_hash_del(&vi->rq[i].napi);
> netif_napi_del(&vi->rq[i].napi);
> }
> - }
Those removing of braces seems unrelated to the topic, please drop them.
>
> remove_vq_common(vi);
>
> flush_work(&vi->config_work);
> -
> - return 0;
> }
>
> -static int virtnet_restore(struct virtio_device *vdev)
> +static int virtnet_start(struct virtnet_info *vi)
> {
> - struct virtnet_info *vi = vdev->priv;
> int err, i;
>
> err = init_vqs(vi);
> @@ -1944,7 +2125,27 @@ static int virtnet_restore(struct virtio_device *vdev)
> mutex_lock(&vi->config_lock);
> vi->config_enable = true;
> mutex_unlock(&vi->config_lock);
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int virtnet_freeze(struct virtio_device *vdev)
> +{
> + struct virtnet_info *vi = vdev->priv;
>
> + unregister_hotcpu_notifier(&vi->nb);
> + virtnet_stop(vi);
> + return 0;
> +}
> +
> +static int virtnet_restore(struct virtio_device *vdev)
> +{
> + struct virtnet_info *vi = vdev->priv;
> + int err;
> +
> + err = virtnet_start(vi);
> + if (err)
> + return err;
> rtnl_lock();
> virtnet_set_queues(vi, vi->curr_queue_pairs);
> rtnl_unlock();
> @@ -1957,6 +2158,22 @@ static int virtnet_restore(struct virtio_device *vdev)
> }
> #endif
>
> +static int virtnet_reset(struct virtnet_info *vi, u64 *data)
> +{
> + struct virtio_device *vdev = vi->vdev;
> + u8 status;
> +
> + virtnet_stop(vi);
> + virtio_feature_negotiate(vdev);
> + *data = virtnet_start(vi);
> + if (*data)
> + return *data;
> + virtnet_set_queues(vi, vi->curr_queue_pairs);
> + status = vdev->config->get_status(vdev);
> + vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_DRIVER_OK);
> + return 0;
> +}
> +
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
> { 0 },
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index fed0ce1..2fc396c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -117,11 +117,10 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev,
> }
> EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
>
> -static int virtio_dev_probe(struct device *_d)
> +int virtio_feature_negotiate(struct virtio_device *dev)
> {
> - int err, i;
> - struct virtio_device *dev = dev_to_virtio(_d);
> struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> + int i;
> u32 device_features;
>
> /* We have a driver! */
> @@ -134,7 +133,8 @@ static int virtio_dev_probe(struct device *_d)
> memset(dev->features, 0, sizeof(dev->features));
> for (i = 0; i < drv->feature_table_size; i++) {
> unsigned int f = drv->feature_table[i];
> - BUG_ON(f >= 32);
> + if (f >= 32)
> + return -EINVAL;
> if (device_features & (1 << f))
> set_bit(f, dev->features);
> }
> @@ -145,7 +145,19 @@ static int virtio_dev_probe(struct device *_d)
> set_bit(i, dev->features);
>
> dev->config->finalize_features(dev);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(virtio_feature_negotiate);
>
> +static int virtio_dev_probe(struct device *_d)
Need another patch for this.
> +{
> + int err;
> + struct virtio_device *dev = dev_to_virtio(_d);
> + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> +
> + err = virtio_feature_negotiate(dev);
> + if (err)
> + return err;
> err = drv->probe(dev);
> if (err)
> add_status(dev, VIRTIO_CONFIG_S_FAILED);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index b46671e..49d8ab4 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -98,6 +98,8 @@ struct virtio_device {
> void *priv;
> };
>
> +int virtio_feature_negotiate(struct virtio_device *dev);
> +
> static inline struct virtio_device *dev_to_virtio(struct device *_dev)
> {
> return container_of(_dev, struct virtio_device, dev);
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 172a7f0..1f31f90 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -201,4 +201,13 @@ struct virtio_net_ctrl_mq {
> #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN 1
> #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX 0x8000
>
> + /*
> + * Control Loopback(5 is used by VIRTIO_NET_CTRL_GUEST_OFFLOADS in latest qemu)
> + *
> + * The command VIRTIO_NET_CTRL_LOOPBACK_SET is used to require the device come
> + * into loopback state.
> + */
> +#define VIRTIO_NET_CTRL_LOOPBACK 6
> + #define VIRTIO_NET_CTRL_LOOPBACK_SET 0
> + #define VIRTIO_NET_CTRL_LOOPBACK_UNSET 1
> #endif /* _LINUX_VIRTIO_NET_H */
^ permalink raw reply
* Re: [PATCH 3/3] virtio_ring: unify direct/indirect code paths.
From: Rusty Russell @ 2014-09-05 2:55 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: netdev, Linux Virtualization, Michael S. Tsirkin
In-Reply-To: <CALCETrVCR+Zm9RtF7gGmO-XcMAb4fLsxKMG-aEABqhbywF9+Jw@mail.gmail.com>
Andy Lutomirski <luto@amacapital.net> writes:
> On Tue, Sep 2, 2014 at 9:29 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> virtqueue_add() populates the virtqueue descriptor table from the sgs
>> given. If it uses an indirect descriptor table, then it puts a single
>> descriptor in the descriptor table pointing to the kmalloc'ed indirect
>> table where the sg is populated.
>>
>> Previously vring_add_indirect() did the allocation and the simple
>> linear layout. We replace that with alloc_indirect() which allocates
>> the indirect table then chains it like the normal descriptor table so
>> we can reuse the core logic.
>>
>
>> + if (vq->indirect && total_sg > 1 && vq->vq.num_free)
>> + desc = alloc_indirect(total_sg, gfp);
>> + else
>> + desc = NULL;
>> +
>> + if (desc) {
>> + /* Use a single buffer which doesn't continue */
>> + vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT;
>> + vq->vring.desc[head].addr = virt_to_phys(desc);
>> + /* avoid kmemleak false positive (hidden by virt_to_phys) */
>> + kmemleak_ignore(desc);
>> + vq->vring.desc[head].len = total_sg * sizeof(struct vring_desc);
>> +
>> + /* Set up rest to use this indirect table. */
>> + i = 0;
>> + total_sg = 1;
>
> This is a little too magical for me. Would it make sense to add a new
> variable for this (total_root_descs or something)?
Agreed, it's a little hacky.
Here's the diff (I actually merged this into the patch, but no point
re-xmitting):
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a4ebbffac141..6d2b5310c991 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -131,7 +131,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
struct vring_virtqueue *vq = to_vvq(_vq);
struct scatterlist *sg;
struct vring_desc *desc;
- unsigned int i, n, avail, uninitialized_var(prev);
+ unsigned int i, n, avail, descs_used, uninitialized_var(prev);
int head;
bool indirect;
@@ -179,17 +179,18 @@ static inline int virtqueue_add(struct virtqueue *_vq,
/* Set up rest to use this indirect table. */
i = 0;
- total_sg = 1;
+ descs_used = 1;
indirect = true;
} else {
desc = vq->vring.desc;
i = head;
+ descs_used = total_sg;
indirect = false;
}
- if (vq->vq.num_free < total_sg) {
+ if (vq->vq.num_free < descs_used) {
pr_debug("Can't add buf len %i - avail = %i\n",
- total_sg, vq->vq.num_free);
+ descs_used, vq->vq.num_free);
/* FIXME: for historical reasons, we force a notify here if
* there are outgoing parts to the buffer. Presumably the
* host should service the ring ASAP. */
@@ -200,7 +201,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
}
/* We're about to use some buffers from the free list. */
- vq->vq.num_free -= total_sg;
+ vq->vq.num_free -= descs_used;
for (n = 0; n < out_sgs; n++) {
for (sg = sgs[n]; sg; sg = sg_next(sg)) {
^ permalink raw reply related
* Re: [PATCH] pktgen: nowait parameter.
From: Jason Wang @ 2014-09-05 2:52 UTC (permalink / raw)
To: Rusty Russell, Jesper Dangaard Brouer
Cc: netdev, Mathias Krause, Robert Olsson
In-Reply-To: <87sik6kfa8.fsf@rustcorp.com.au>
On 09/05/2014 09:49 AM, Rusty Russell wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>> > On Wed, 03 Sep 2014 13:50:01 +0930
>> > Rusty Russell <rusty@rustcorp.com.au> wrote:
>> >
>>> >> While trying to measure speed of virtio_net, I was getting hangs.
>>> >> This is because we skb_orphan() but delay the tx interrupt
>>> >> indefinitely (by number of slots).
>>> >>
>>> >> With nowait, pktgen won't wait for the skb to be released. This
>>> >> introduces an error, but it's ok if count >> ringsize.
>> >
>> > This pktgen_wait_for_skb() only happens it the exit case, when count
>> > packets have been send. I guess its okay to proceed to
>> > pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with
>> > refcnt=2, decrementing to refcnt=1, and then we depend on driver to
>> > eventually call kfree_skb().
> Yes, exactly.
>
>>> >> I updated the documentation, but it needs far more work (it
>>> >> refers to pgset and an examples directory, none of which exist
>>> >> in the kernel tree).
>> >
>> > Yes, the doc is not in such a good shape.
>> >
>> > I'm not 100% happy with the name "nowait" parameter, as users could
>> > easily misunderstand the purpose of this parameter. But I've not come
>> > up with a better name, e.g. "exit_nowait" is also not the best.
> Agreed. It could also be a flag, though that doesn't help with the name.
>
>> > diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>>> >> index 8b849ddfef2e..adc41f2b3bc7 100644
>>> >> --- a/net/core/pktgen.c
>>> >> +++ b/net/core/pktgen.c
>>> >> @@ -290,6 +290,11 @@ struct pktgen_dev {
>>> >> * set clone_skb to 1024.
>>> >> */
>>> >>
>>> >> + bool no_wait; /*
>>> >> + * Don't wait for packet to be freed
>>> >> + * by driver
>>> >> + */
>>> >> +
>> >
>> > DaveM prefers multi line comments like:
>> >
>> > /* Don't wait for packet to be freed
>> > * by driver
>> > */
> He does, but the rest of the kernel and the comment immediately above
> doesn't:
>
> int clone_skb; /*
> * Use multiple SKBs during packet gen.
> * If this number is greater than 1, then
> * that many copies of the same packet will be
> * sent before a new packet is allocated.
> * If you want to send 1024 identical packets
> * before creating a new packet,
> * set clone_skb to 1024.
> */
>
>>> >> char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>>> >> char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>>> >> char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>>> >> @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
>>> >>
>>> >> seq_puts(seq, "\n");
>>> >>
>>> >> + if (pkt_dev->no_wait)
>>> >> + seq_puts(seq, " nowait\n");
>>> >> +
>> >
>> > Shouldn't you put this print statement above the "Flags:" section?
> Sure.
>
>>> >> /* not really stopped, more like last-running-at */
>>> >> stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at;
>>> >> idle = pkt_dev->idle_acc;
>>> >> @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file,
> Subject: pktgen: nowait parameter.
>
> While trying to measure speed of virtio_net, I was getting hangs.
> This is because we skb_orphan() but delay the tx interrupt
> indefinitely (by number of slots).
>
> With nowait, pktgen won't wait for the skb to be released. This
> introduces an error, but it's ok if count >> ringsize.
>
> I updated the documentation, but it needs far more work (it
> refers to pgset and an examples directory, none of which exist
> in the kernel tree).
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This depends on user to know the internals of driver (e.g whether the tx
completion is delayed by something e.g using skb_orphan()) which may not
be a good idea. We may change the virtio-net to use tx interrupt in the
future (I'm testing a draft patch to do this). How about something
transparent to the user? I post a patch that marking such device with a
special flag (https://patchwork.kernel.org/patch/1800711/), but not all
like it. Maybe we need a new ndo_tx_polling() for pktgen or someone else
to poll the tx completion?
Thanks
^ permalink raw reply
* Re: [PATCH] pktgen: nowait parameter.
From: Rusty Russell @ 2014-09-05 1:49 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Mathias Krause, brouer, Robert Olsson
In-Reply-To: <20140903110956.0872841f@redhat.com>
Jesper Dangaard Brouer <brouer@redhat.com> writes:
> On Wed, 03 Sep 2014 13:50:01 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> While trying to measure speed of virtio_net, I was getting hangs.
>> This is because we skb_orphan() but delay the tx interrupt
>> indefinitely (by number of slots).
>>
>> With nowait, pktgen won't wait for the skb to be released. This
>> introduces an error, but it's ok if count >> ringsize.
>
> This pktgen_wait_for_skb() only happens it the exit case, when count
> packets have been send. I guess its okay to proceed to
> pktgen_stop_device() which will call kfree_skb(pkt_dev->skb) with
> refcnt=2, decrementing to refcnt=1, and then we depend on driver to
> eventually call kfree_skb().
Yes, exactly.
>> I updated the documentation, but it needs far more work (it
>> refers to pgset and an examples directory, none of which exist
>> in the kernel tree).
>
> Yes, the doc is not in such a good shape.
>
> I'm not 100% happy with the name "nowait" parameter, as users could
> easily misunderstand the purpose of this parameter. But I've not come
> up with a better name, e.g. "exit_nowait" is also not the best.
Agreed. It could also be a flag, though that doesn't help with the name.
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
>> index 8b849ddfef2e..adc41f2b3bc7 100644
>> --- a/net/core/pktgen.c
>> +++ b/net/core/pktgen.c
>> @@ -290,6 +290,11 @@ struct pktgen_dev {
>> * set clone_skb to 1024.
>> */
>>
>> + bool no_wait; /*
>> + * Don't wait for packet to be freed
>> + * by driver
>> + */
>> +
>
> DaveM prefers multi line comments like:
>
> /* Don't wait for packet to be freed
> * by driver
> */
He does, but the rest of the kernel and the comment immediately above
doesn't:
int clone_skb; /*
* Use multiple SKBs during packet gen.
* If this number is greater than 1, then
* that many copies of the same packet will be
* sent before a new packet is allocated.
* If you want to send 1024 identical packets
* before creating a new packet,
* set clone_skb to 1024.
*/
>> char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>> char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>> char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
>> @@ -679,6 +684,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
>>
>> seq_puts(seq, "\n");
>>
>> + if (pkt_dev->no_wait)
>> + seq_puts(seq, " nowait\n");
>> +
>
> Shouldn't you put this print statement above the "Flags:" section?
Sure.
>> /* not really stopped, more like last-running-at */
>> stopped = pkt_dev->running ? ktime_get() : pkt_dev->stopped_at;
>> idle = pkt_dev->idle_acc;
>> @@ -1711,6 +1719,17 @@ static ssize_t pktgen_if_write(struct file *file,
Subject: pktgen: nowait parameter.
While trying to measure speed of virtio_net, I was getting hangs.
This is because we skb_orphan() but delay the tx interrupt
indefinitely (by number of slots).
With nowait, pktgen won't wait for the skb to be released. This
introduces an error, but it's ok if count >> ringsize.
I updated the documentation, but it needs far more work (it
refers to pgset and an examples directory, none of which exist
in the kernel tree).
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/Documentation/networking/pktgen.txt b/Documentation/networking/pktgen.txt
index 0dffc6e37902..dbd993d1e7f8 100644
--- a/Documentation/networking/pktgen.txt
+++ b/Documentation/networking/pktgen.txt
@@ -41,10 +41,13 @@ NIC HW layer (which is bad for bufferbloat).
One should be careful to conclude, that packets/descriptors in the HW
TX ring cause delay. Drivers usually delay cleaning up the
ring-buffers (for various performance reasons), thus packets stalling
-the TX ring, might just be waiting for cleanup.
+the TX ring, might just be waiting for cleanup. Writing the "nowait"
+parameter into /proc/net/pktgen/ethX will avoid waiting for cleanup of
+the final packets, introducing a slight error (tiny if the count of
+packets being sent is much greater than the ring size of the device).
-This cleanup issues is specifically the case, for the driver ixgbe
-(Intel 82599 chip). This driver (ixgbe) combine TX+RX ring cleanups,
+Alternately, some drivers (eg ixgbe for the Intel 82599 chip) can
+have their cleanup interval changed. ixgbe combines TX+RX ring cleanups,
and the cleanup interval is affected by the ethtool --coalesce setting
of parameter "rx-usecs".
@@ -303,6 +306,8 @@ flowlen
rate
ratep
+nowait
+
References:
ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/
ftp://robur.slu.se/pub/Linux/net-development/pktgen-testing/examples/
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 8b849ddfef2e..1589f5ac0509 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -290,6 +290,8 @@ struct pktgen_dev {
* set clone_skb to 1024.
*/
+ bool no_wait; /* Don't wait for packet to be freed by driver */
+
char dst_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
char dst_max[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
char src_min[IP_NAME_SZ]; /* IP, ie 1.2.3.4 */
@@ -615,6 +617,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
if (pkt_dev->node >= 0)
seq_printf(seq, " node: %d\n", pkt_dev->node);
+ if (pkt_dev->no_wait)
+ seq_puts(seq, " nowait\n");
+
seq_puts(seq, " Flags: ");
if (pkt_dev->flags & F_IPV6)
@@ -1711,6 +1716,17 @@ static ssize_t pktgen_if_write(struct file *file,
return count;
}
+ if (!strcmp(name, "nowait")) {
+ len = num_arg(&user_buffer[i], 10, &value);
+ if (len < 0)
+ return len;
+
+ i += len;
+ pkt_dev->no_wait = value;
+ sprintf(pg_result, "OK: nowait=%u", pkt_dev->no_wait);
+ return count;
+ }
+
sprintf(pkt_dev->result, "No such parameter \"%s\"", name);
return -EINVAL;
}
@@ -3373,7 +3389,8 @@ unlock:
/* If pkt_dev->count is zero, then run forever */
if ((pkt_dev->count != 0) && (pkt_dev->sofar >= pkt_dev->count)) {
- pktgen_wait_for_skb(pkt_dev);
+ if (!pkt_dev->no_wait)
+ pktgen_wait_for_skb(pkt_dev);
/* Done with this */
pktgen_stop_device(pkt_dev);
@@ -3565,6 +3582,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
pkt_dev->svlan_cfi = 0;
pkt_dev->svlan_id = 0xffff;
pkt_dev->node = -1;
+ pkt_dev->no_wait = false;
err = pktgen_setup_dev(t->net, pkt_dev, ifname);
if (err)
^ permalink raw reply related
* [PATCH 1/1] add selftest for virtio-net v1.0
From: Hengjinxiao @ 2014-09-05 1:51 UTC (permalink / raw)
To: virtualization, netdev, linux-kernel, linux-api; +Cc: famz, mst
Selftest is an important part of network driver, this patch adds selftest for
virtio-net, including loopback test, negotiate test and reset test. Loopback
test checks whether virtio-net can send and receive packets normally. Negotiate test
executes feature negotiation between virtio-net driver in Guest OS and virtio-net
device in Host OS. Reset test resets virtio-net.
Following last patch, this version has deleted some useless codes and fixed bugs
as you suggest.
Any corrections are welcome.
Signed-off-by: Hengjinxiao <hjxiaohust@gmail.com>
---
drivers/net/virtio_net.c | 241 ++++++++++++++++++++++++++++++++++++++--
drivers/virtio/virtio.c | 20 +++-
include/linux/virtio.h | 2 +
include/uapi/linux/virtio_net.h | 9 ++
4 files changed, 256 insertions(+), 16 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 59caa06..22d8228 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -28,6 +28,7 @@
#include <linux/cpu.h>
#include <linux/average.h>
#include <net/busy_poll.h>
+#include <linux/pci.h>
static int napi_weight = NAPI_POLL_WEIGHT;
module_param(napi_weight, int, 0444);
@@ -51,6 +52,17 @@ module_param(gso, bool, 0444);
#define MERGEABLE_BUFFER_ALIGN max(L1_CACHE_BYTES, 256)
#define VIRTNET_DRIVER_VERSION "1.0.0"
+#define __VIRTNET_TESTING 0
+
+static const struct {
+ const char string[ETH_GSTRING_LEN];
+} virtnet_gstrings_test[] = {
+ { "loopback test (offline)" },
+ { "negotiate test (offline)" },
+ { "reset test (offline)" },
+};
+
+#define VIRTNET_NUM_TEST ARRAY_SIZE(virtnet_gstrings_test)
struct virtnet_stats {
struct u64_stats_sync tx_syncp;
@@ -104,6 +116,8 @@ struct virtnet_info {
struct send_queue *sq;
struct receive_queue *rq;
unsigned int status;
+ unsigned long flags;
+ atomic_t lb_count;
/* Max # of queue pairs supported by the device */
u16 max_queue_pairs;
@@ -436,6 +450,19 @@ err_buf:
return NULL;
}
+void virtnet_check_lb_frame(struct virtnet_info *vi,
+ struct sk_buff *skb)
+{
+ unsigned int frame_size = skb->len;
+
+ if (*(skb->data + 3) == 0xFF) {
+ if ((*(skb->data + frame_size / 2 + 10) == 0xBE) &&
+ (*(skb->data + frame_size / 2 + 12) == 0xAF)) {
+ atomic_dec(&vi->lb_count);
+ }
+ }
+}
+
static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
{
struct virtnet_info *vi = rq->vq->vdev->priv;
@@ -485,7 +512,12 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
} else if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID) {
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
-
+ /* loopback self test for ethtool */
+ if (test_bit(__VIRTNET_TESTING, &vi->flags)) {
+ virtnet_check_lb_frame(vi, skb);
+ dev_kfree_skb_any(skb);
+ return;
+ }
skb->protocol = eth_type_trans(skb, dev);
pr_debug("Receiving skb proto 0x%04x len %i type %i\n",
ntohs(skb->protocol), skb->len, skb->pkt_type);
@@ -813,6 +845,9 @@ static int virtnet_open(struct net_device *dev)
{
struct virtnet_info *vi = netdev_priv(dev);
int i;
+ /* disallow open during test */
+ if (test_bit(__VIRTNET_TESTING, &vi->flags))
+ return -EBUSY;
for (i = 0; i < vi->max_queue_pairs; i++) {
if (i < vi->curr_queue_pairs)
@@ -1363,12 +1398,166 @@ static void virtnet_get_channels(struct net_device *dev,
channels->other_count = 0;
}
+static int virtnet_reset(struct virtnet_info *vi, u64 *data);
+
+static void virtnet_create_lb_frame(struct sk_buff *skb,
+ unsigned int frame_size)
+{
+ memset(skb->data, 0xFF, frame_size);
+ frame_size &= ~1;
+ memset(&skb->data[frame_size / 2], 0xAA, frame_size / 2 - 1);
+ memset(&skb->data[frame_size / 2 + 10], 0xBE, 1);
+ memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
+}
+
+static int virtnet_start_loopback(struct virtnet_info *vi)
+{
+ if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_LOOPBACK,
+ VIRTIO_NET_CTRL_LOOPBACK_SET, NULL, NULL)) {
+ dev_warn(&vi->dev->dev, "Failed to set loopback.\n");
+ return -EINVAL;
+ }
+ for (i = 0; i < vi->curr_queue_pairs; i++)
+ napi_disable(&vi->rq[i].napi);
+ return 0;
+}
+
+static int virtnet_run_loopback_test(struct virtnet_info *vi)
+{
+ int i;
+ struct sk_buff *skb;
+ unsigned int size = GOOD_COPY_LEN;
+
+ for (i = 0; i < 100; i++) {
+ skb = netdev_alloc_skb(vi->dev, size);
+ if (!skb)
+ return -ENOMEM;
+
+ skb->queue_mapping = 0;
+ skb_put(skb, size);
+ virtnet_create_lb_frame(skb, size);
+ start_xmit(skb, vi->dev);
+ atomic_inc(&vi->lb_count);
+ }
+ free_old_xmit_skbs(&vi->sq[skb->queue_mapping]);
+ /* Give queue time to settle before testing results. */
+ msleep(20);
+ for (i = 0; i < vi->curr_queue_pairs; i++) {
+ void *buf;
+ unsigned int len, received = 0;
+
+ while ((received < 100) &&
+ (buf = virtqueue_get_buf(vi->rq[i].vq, &len)) != NULL) {
+ receive_buf(&vi->rq[i], buf, len);
+ --vi->rq[i].num;
+ received++;
+ }
+ if ((vi->rq[i].vq)->num_free <
+ virtqueue_get_vring_size(vi->rq[i].vq) / 2)
+ if (!try_fill_recv(&vi->rq[i], GFP_ATOMIC))
+ schedule_delayed_work(&vi->refill, 0);
+ }
+ return atomic_read(&vi->lb_count) ? -EIO : 0;
+}
+
+static int virtnet_stop_loopback(struct virtnet_info *vi)
+{
+ int i;
+
+ for (i = 0; i < vi->curr_queue_pairs; i++)
+ virtnet_napi_enable(&vi->rq[i]);
+ if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_LOOPBACK,
+ VIRTIO_NET_CTRL_LOOPBACK_UNSET, NULL, NULL)) {
+ dev_warn(&vi->dev->dev, "Failed to unset loopback.\n");
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int virtnet_loopback_test(struct virtnet_info *vi, u64 *data)
+{
+ *data = virtnet_start_loopback(vi);
+ if (*data)
+ goto out;
+ *data = virtnet_run_loopback_test(vi);
+ if (*data)
+ goto out;
+ *data = virtnet_stop_loopback(vi);
+out:
+ return *data;
+}
+
+static int virtnet_feature_neg_test(struct virtnet_info *vi, u64 *data)
+{
+ struct virtio_device *dev = vi->vdev;
+ u8 status;
+
+ status = dev->config->get_status(dev);
+ if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+ u8 test_status = status & ~VIRTIO_CONFIG_S_DRIVER_OK;
+
+ dev->config->set_status(dev, test_status);
+ }
+ *data = virtio_feature_negotiate(dev);
+ dev->config->set_status(dev, status);
+ return *data;
+}
+
+static int virtnet_get_sset_count(struct net_device *netdev, int sset)
+{
+ switch (sset) {
+ case ETH_SS_TEST:
+ return VIRTNET_NUM_TEST;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static void virtnet_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
+{
+ switch (stringset) {
+ case ETH_SS_TEST:
+ memcpy(buf, &virtnet_gstrings_test,
+ sizeof(virtnet_gstrings_test));
+ break;
+ default:
+ break;
+ }
+}
+
+static void virtnet_self_test(struct net_device *netdev,
+ struct ethtool_test *eth_test, u64 *data)
+{
+ struct virtnet_info *vi = netdev_priv(netdev);
+
+ memset(data, 0, sizeof(u64) * VIRTNET_NUM_TEST);
+ if (netif_running(netdev)) {
+ set_bit(__VIRTNET_TESTING, &vi->flags);
+ if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
+ if (virtnet_loopback_test(vi, &data[0]))
+ eth_test->flags |= ETH_TEST_FL_FAILED;
+ if (virtnet_feature_neg_test(vi, &data[1]))
+ eth_test->flags |= ETH_TEST_FL_FAILED;
+ if (virtnet_reset(vi, &data[2]))
+ eth_test->flags |= ETH_TEST_FL_FAILED;
+ }
+ clear_bit(__VIRTNET_TESTING, &vi->flags);
+ } else {
+ dev_warn(&vi->dev->dev,
+ "%s is down, Loopback test will fail.\n", netdev->name);
+ eth_test->flags |= ETH_TEST_FL_FAILED;
+ }
+}
+
static const struct ethtool_ops virtnet_ethtool_ops = {
.get_drvinfo = virtnet_get_drvinfo,
.get_link = ethtool_op_get_link,
.get_ringparam = virtnet_get_ringparam,
.set_channels = virtnet_set_channels,
.get_channels = virtnet_get_channels,
+ .self_test = virtnet_self_test,
+ .get_strings = virtnet_get_strings,
+ .get_sset_count = virtnet_get_sset_count,
};
#define MIN_MTU 68
@@ -1890,14 +2079,10 @@ static void virtnet_remove(struct virtio_device *vdev)
free_netdev(vi->dev);
}
-#ifdef CONFIG_PM_SLEEP
-static int virtnet_freeze(struct virtio_device *vdev)
+static void virtnet_stop(struct virtnet_info *vi)
{
- 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;
@@ -1906,24 +2091,20 @@ static int virtnet_freeze(struct virtio_device *vdev)
netif_device_detach(vi->dev);
cancel_delayed_work_sync(&vi->refill);
- if (netif_running(vi->dev)) {
+ if (netif_running(vi->dev))
for (i = 0; i < vi->max_queue_pairs; i++) {
napi_disable(&vi->rq[i].napi);
napi_hash_del(&vi->rq[i].napi);
netif_napi_del(&vi->rq[i].napi);
}
- }
remove_vq_common(vi);
flush_work(&vi->config_work);
-
- return 0;
}
-static int virtnet_restore(struct virtio_device *vdev)
+static int virtnet_start(struct virtnet_info *vi)
{
- struct virtnet_info *vi = vdev->priv;
int err, i;
err = init_vqs(vi);
@@ -1944,7 +2125,27 @@ static int virtnet_restore(struct virtio_device *vdev)
mutex_lock(&vi->config_lock);
vi->config_enable = true;
mutex_unlock(&vi->config_lock);
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int virtnet_freeze(struct virtio_device *vdev)
+{
+ struct virtnet_info *vi = vdev->priv;
+ unregister_hotcpu_notifier(&vi->nb);
+ virtnet_stop(vi);
+ return 0;
+}
+
+static int virtnet_restore(struct virtio_device *vdev)
+{
+ struct virtnet_info *vi = vdev->priv;
+ int err;
+
+ err = virtnet_start(vi);
+ if (err)
+ return err;
rtnl_lock();
virtnet_set_queues(vi, vi->curr_queue_pairs);
rtnl_unlock();
@@ -1957,6 +2158,22 @@ static int virtnet_restore(struct virtio_device *vdev)
}
#endif
+static int virtnet_reset(struct virtnet_info *vi, u64 *data)
+{
+ struct virtio_device *vdev = vi->vdev;
+ u8 status;
+
+ virtnet_stop(vi);
+ virtio_feature_negotiate(vdev);
+ *data = virtnet_start(vi);
+ if (*data)
+ return *data;
+ virtnet_set_queues(vi, vi->curr_queue_pairs);
+ status = vdev->config->get_status(vdev);
+ vdev->config->set_status(vdev, status | VIRTIO_CONFIG_S_DRIVER_OK);
+ return 0;
+}
+
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
{ 0 },
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index fed0ce1..2fc396c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -117,11 +117,10 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev,
}
EXPORT_SYMBOL_GPL(virtio_check_driver_offered_feature);
-static int virtio_dev_probe(struct device *_d)
+int virtio_feature_negotiate(struct virtio_device *dev)
{
- int err, i;
- struct virtio_device *dev = dev_to_virtio(_d);
struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+ int i;
u32 device_features;
/* We have a driver! */
@@ -134,7 +133,8 @@ static int virtio_dev_probe(struct device *_d)
memset(dev->features, 0, sizeof(dev->features));
for (i = 0; i < drv->feature_table_size; i++) {
unsigned int f = drv->feature_table[i];
- BUG_ON(f >= 32);
+ if (f >= 32)
+ return -EINVAL;
if (device_features & (1 << f))
set_bit(f, dev->features);
}
@@ -145,7 +145,19 @@ static int virtio_dev_probe(struct device *_d)
set_bit(i, dev->features);
dev->config->finalize_features(dev);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(virtio_feature_negotiate);
+static int virtio_dev_probe(struct device *_d)
+{
+ int err;
+ struct virtio_device *dev = dev_to_virtio(_d);
+ struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
+
+ err = virtio_feature_negotiate(dev);
+ if (err)
+ return err;
err = drv->probe(dev);
if (err)
add_status(dev, VIRTIO_CONFIG_S_FAILED);
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index b46671e..49d8ab4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -98,6 +98,8 @@ struct virtio_device {
void *priv;
};
+int virtio_feature_negotiate(struct virtio_device *dev);
+
static inline struct virtio_device *dev_to_virtio(struct device *_dev)
{
return container_of(_dev, struct virtio_device, dev);
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 172a7f0..1f31f90 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -201,4 +201,13 @@ struct virtio_net_ctrl_mq {
#define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN 1
#define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX 0x8000
+ /*
+ * Control Loopback(5 is used by VIRTIO_NET_CTRL_GUEST_OFFLOADS in latest qemu)
+ *
+ * The command VIRTIO_NET_CTRL_LOOPBACK_SET is used to require the device come
+ * into loopback state.
+ */
+#define VIRTIO_NET_CTRL_LOOPBACK 6
+ #define VIRTIO_NET_CTRL_LOOPBACK_SET 0
+ #define VIRTIO_NET_CTRL_LOOPBACK_UNSET 1
#endif /* _LINUX_VIRTIO_NET_H */
--
1.8.3.2
^ permalink raw reply related
* [PATCH net v6 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: Benjamin Poirier @ 2014-09-05 1:30 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409880647-14887-1-git-send-email-bpoirier@suse.de>
In tg3_set_ringparam(), the tx_pending test to cover the cases where
tg3_tso_bug() is entered has two problems
1) the check is only done for certain hardware whereas the workaround
is now used more broadly. IOW, the check may not be performed when it
is needed.
2) the check is too optimistic.
For example, with a 5761 (SHORT_DMA_BUG), tg3_set_ringparam() skips over the
"tx_pending <= (MAX_SKB_FRAGS * 3)" check because TSO_BUG is false. Even if it
did do the check, with a full sized skb, frag_cnt_est = 135 but the check is
for <= MAX_SKB_FRAGS * 3 (= 17 * 3 = 51). So the check is insufficient. This
leads to the following situation: by setting, ex. tx_pending = 100, there can
be an skb that triggers tg3_tso_bug() and that is large enough to cause
tg3_tso_bug() to stop the queue even when it is empty. We then end up with a
netdev watchdog transmit timeout.
Given that 1) some of the conditions tested for in tg3_tx_frag_set() apply
regardless of the chipset flags and that 2) it is difficult to estimate ahead
of time the max possible number of frames that a large skb may be split into
by gso, this patch changes tg3_set_ringparam() to ignore the requirements of
tg3_tso_bug(). Those requirements are instead checked in tg3_tso_bug() itself
and if there is not a sufficient number of descriptors available in the tx
queue, the skb is linearized.
This patch also removes the current scheme in tg3_tso_bug() where the number
of descriptors required to transmit an skb is estimated. Instead,
gso_segment() is called without _SG which yields predictable, linear skbs.
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v1->v2
* in tg3_set_ringparam(), reduce gso_max_segs further to budget 3 descriptors
per gso seg instead of only 1 as in v1
* in tg3_tso_bug(), check that this estimation (3 desc/seg) holds, otherwise
linearize some skbs as needed
* in tg3_start_xmit(), make the queue stop threshold a parameter, for the
reason explained in the commit description
Changes v2->v3
* use tg3_maybe_stop_txq() instead of repeatedly open coding it
* add the requested tp->tx_dropped++ stat increase in tg3_tso_bug() if
skb_linearize() fails and we must abort
* in the same code block, add an additional check to stop the queue with the
default threshold. Otherwise, the netdev_err message at the start of
__tg3_start_xmit() could be triggered when the next frame is transmitted.
That is because the previous calls to __tg3_start_xmit() in tg3_tso_bug()
may have been using a stop_thresh=segs_remaining that is < MAX_SKB_FRAGS +
1.
Changes v3->v4
* in tg3_set_ringparam(), make sure that wakeup_thresh does not end up being
>= tx_pending. Identified by Prashant.
Changes v4->v5
* in tg3_set_ringparam(), use TG3_TX_WAKEUP_THRESH() and tp->txq_cnt instead
of tp->irq_max. Identified by Prashant.
Changes v5->v6
* avoid changing gso_max_segs and making the tx queue wakeup threshold
dynamic. Instead of stopping the queue when there are not enough descriptors
available, the skb is linearized.
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending <= 135
---
drivers/net/ethernet/broadcom/tg3.c | 59 ++++++++++++++++++++++++-------------
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 6e6b07c..a9787a1 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7830,6 +7830,8 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi *tnapi,
}
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *, struct net_device *,
+ u32);
/* Returns true if the queue has been stopped. Note that it may have been
* restarted since.
@@ -7866,27 +7868,38 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
- struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)->gso_segs;
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
- if (netif_tx_queue_stopped(txq))
- return NETDEV_TX_BUSY;
+ if (unlikely(tg3_tx_avail(tnapi) <= segs_remaining)) {
+ if (!skb_is_nonlinear(skb) || skb_linearize(skb))
+ goto tg3_tso_bug_drop;
+ tg3_start_xmit(skb, tp->dev);
+ } else {
+ struct sk_buff *segs, *nskb;
- segs = skb_gso_segment(skb, tp->dev->features &
- ~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
- goto tg3_tso_bug_end;
+ segs = skb_gso_segment(skb, tp->dev->features &
+ ~(NETIF_F_TSO | NETIF_F_TSO6 |
+ NETIF_F_SG));
+ if (IS_ERR(segs) || !segs)
+ goto tg3_tso_bug_drop;
- do {
- nskb = segs;
- segs = segs->next;
- nskb->next = NULL;
- tg3_start_xmit(nskb, tp->dev);
- } while (segs);
+ do {
+ nskb = segs;
+ segs = segs->next;
+ nskb->next = NULL;
+ if (--segs_remaining)
+ __tg3_start_xmit(nskb, tp->dev, segs_remaining);
+ else
+ tg3_start_xmit(nskb, tp->dev);
+ } while (segs);
-tg3_tso_bug_end:
+ dev_kfree_skb_any(skb);
+ }
+
+ return NETDEV_TX_OK;
+
+tg3_tso_bug_drop:
+ tp->tx_dropped++;
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
@@ -7895,6 +7908,12 @@ tg3_tso_bug_end:
/* hard_start_xmit for all devices */
static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ return __tg3_start_xmit(skb, dev, MAX_SKB_FRAGS + 1);
+}
+
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
+ struct net_device *dev, u32 stop_thresh)
+{
struct tg3 *tp = netdev_priv(dev);
u32 len, entry, base_flags, mss, vlan = 0;
u32 budget;
@@ -8102,7 +8121,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
tw32_tx_mbox(tnapi->prodmbox, entry);
tnapi->tx_prod = entry;
- tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ tg3_maybe_stop_txq(tnapi, txq, stop_thresh,
TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
@@ -12336,9 +12355,7 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
if ((ering->rx_pending > tp->rx_std_ring_mask) ||
(ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
(ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
- (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
- (tg3_flag(tp, TSO_BUG) &&
- (ering->tx_pending <= (MAX_SKB_FRAGS * 3))))
+ (ering->tx_pending <= MAX_SKB_FRAGS + 1))
return -EINVAL;
if (netif_running(dev)) {
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v6 3/4] tg3: Move tx queue stop logic to its own function
From: Benjamin Poirier @ 2014-09-05 1:30 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409880647-14887-1-git-send-email-bpoirier@suse.de>
It is duplicated. Also, the first instance in tg3_start_xmit() is racy.
Consider:
tg3_start_xmit()
if budget <= ...
tg3_tx()
(free up the entire ring)
tx_cons =
smp_mb
if queue_stopped and tx_avail, NO
if !queue_stopped
stop queue
return NETDEV_TX_BUSY
... tx queue stopped forever
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v2->v3
* new patch to avoid repeatedly open coding this block in the next patch.
Changes v3->v4
* added a comment to clarify the return value, as suggested
* replaced the BUG_ON with netdev_err(). No need to be so dramatic, this
situation will trigger a netdev watchdog anyways.
---
drivers/net/ethernet/broadcom/tg3.c | 75 ++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index c5061c3..6e6b07c 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7831,6 +7831,35 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi *tnapi,
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+/* Returns true if the queue has been stopped. Note that it may have been
+ * restarted since.
+ */
+static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
+ struct netdev_queue *txq,
+ u32 stop_thresh, u32 wakeup_thresh)
+{
+ bool stopped = false;
+
+ if (unlikely(tg3_tx_avail(tnapi) <= stop_thresh)) {
+ if (!netif_tx_queue_stopped(txq)) {
+ stopped = true;
+ netif_tx_stop_queue(txq);
+ if (wakeup_thresh >= tnapi->tx_pending)
+ netdev_err(tnapi->tp->dev,
+ "BUG! wakeup_thresh too large (%u >= %u)\n",
+ wakeup_thresh, tnapi->tx_pending);
+ }
+ /* netif_tx_stop_queue() must be done before checking tx index
+ * in tg3_tx_avail(), because in tg3_tx(), we update tx index
+ * before checking for netif_tx_queue_stopped().
+ */
+ smp_mb();
+ if (tg3_tx_avail(tnapi) > wakeup_thresh)
+ netif_tx_wake_queue(txq);
+ }
+ return stopped;
+}
+
/* Use GSO to workaround all TSO packets that meet HW bug conditions
* indicated in tg3_tx_frag_set()
*/
@@ -7841,20 +7870,9 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
/* Estimate the number of fragments in the worst case */
- if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
- netif_tx_stop_queue(txq);
-
- /* netif_tx_stop_queue() must be done before checking
- * checking tx index in tg3_tx_avail() below, because in
- * tg3_tx(), we update tx index before checking for
- * netif_tx_queue_stopped().
- */
- smp_mb();
- if (tg3_tx_avail(tnapi) <= frag_cnt_est)
- return NETDEV_TX_BUSY;
-
- netif_tx_wake_queue(txq);
- }
+ tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
+ if (netif_tx_queue_stopped(txq))
+ return NETDEV_TX_BUSY;
segs = skb_gso_segment(skb, tp->dev->features &
~(NETIF_F_TSO | NETIF_F_TSO6));
@@ -7902,16 +7920,13 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
* interrupt. Furthermore, IRQ processing runs lockless so we have
* no IRQ context deadlocks to worry about either. Rejoice!
*/
- if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
- if (!netif_tx_queue_stopped(txq)) {
- netif_tx_stop_queue(txq);
-
- /* This is a hard error, log it. */
- netdev_err(dev,
- "BUG! Tx Ring full when queue awake!\n");
- }
- return NETDEV_TX_BUSY;
+ if (tg3_maybe_stop_txq(tnapi, txq, skb_shinfo(skb)->nr_frags + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi))) {
+ /* This is a hard error, log it. */
+ netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
}
+ if (netif_tx_queue_stopped(txq))
+ return NETDEV_TX_BUSY;
entry = tnapi->tx_prod;
base_flags = 0;
@@ -8087,18 +8102,8 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
tw32_tx_mbox(tnapi->prodmbox, entry);
tnapi->tx_prod = entry;
- if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
- netif_tx_stop_queue(txq);
-
- /* netif_tx_stop_queue() must be done before checking
- * checking tx index in tg3_tx_avail() below, because in
- * tg3_tx(), we update tx index before checking for
- * netif_tx_queue_stopped().
- */
- smp_mb();
- if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
- netif_tx_wake_queue(txq);
- }
+ tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
return NETDEV_TX_OK;
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v6 2/4] tg3: Fix tx_pending check for MAX_SKB_FRAGS
From: Benjamin Poirier @ 2014-09-05 1:30 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409880647-14887-1-git-send-email-bpoirier@suse.de>
The rest of the driver assumes at least one free descriptor in the tx ring.
Therefore, since an skb with max frags takes up (MAX_SKB_FRAGS + 1)
descriptors, tx_pending must be > (MAX_SKB_FRAGS + 1).
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v1->v2
Moved ahead in the series from 3/3 to 2/3, no functionnal change
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending = 18
---
drivers/net/ethernet/broadcom/tg3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 81b3a57..c5061c3 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12331,7 +12331,7 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
if ((ering->rx_pending > tp->rx_std_ring_mask) ||
(ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
(ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
- (ering->tx_pending <= MAX_SKB_FRAGS) ||
+ (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
(tg3_flag(tp, TSO_BUG) &&
(ering->tx_pending <= (MAX_SKB_FRAGS * 3))))
return -EINVAL;
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v6 1/4] tg3: Limit minimum tx queue wakeup threshold
From: Benjamin Poirier @ 2014-09-05 1:30 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409880647-14887-1-git-send-email-bpoirier@suse.de>
tx_pending may be set by the user (via ethtool -G) to a low enough value that
TG3_TX_WAKEUP_THRESH becomes smaller than MAX_SKB_FRAGS + 1. This may cause
the tx queue to be waked when there are in fact not enough descriptors to
handle an skb with max frags. This in turn causes tg3_start_xmit() to return
NETDEV_TX_BUSY and print error messages. Fix the problem by putting a limit to
how low TG3_TX_WAKEUP_THRESH can go.
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
I noticed the problem in a 3.0 kernel when setting `ethtool eth0 -G tx 50` and
running a netperf TCP_STREAM test. The console fills up with
[10597.596155] tg3 0000:06:00.0: eth0: BUG! Tx Ring full when queue awake!
The problem in tg3 remains in current kernels though it does not reproduce as
easily since "5640f76 net: use a per task frag allocator (v3.7-rc1)". I
reproduced on current kernels by using the fail_page_alloc fault injection
mechanism to force the creation of skbs with many order-0 frags. Note that the
following script may also trigger another bug (NETDEV WATCHDOG), which is
fixed in the next patch.
$ cat /tmp/doit.sh
F="/sys/kernel/debug/fail_page_alloc"
echo -1 > "$F/times"
echo 0 > "$F/verbose"
echo 0 > "$F/ignore-gfp-wait"
echo 1 > "$F/task-filter"
echo 100 > "$F/probability"
netperf -H 192.168.9.30 -l100 -t omni -- -d send &
n=$!
sleep 0.3
echo 1 > "/proc/$n/make-it-fail"
sleep 10
kill "$n"
---
drivers/net/ethernet/broadcom/tg3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index cb77ae9..81b3a57 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS flag, unsigned long *bits)
#endif
/* minimum number of free TX descriptors required to wake up TX process */
-#define TG3_TX_WAKEUP_THRESH(tnapi) ((tnapi)->tx_pending / 4)
+#define TG3_TX_WAKEUP_THRESH(tnapi) max_t(u32, (tnapi)->tx_pending / 4, \
+ MAX_SKB_FRAGS + 1)
#define TG3_TX_BD_DMA_MAX_2K 2048
#define TG3_TX_BD_DMA_MAX_4K 4096
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v6 0/4] tg3: tx_pending fixes
From: Benjamin Poirier @ 2014-09-05 1:30 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
Extra info regarding patch 4:
This version of the series calls gso_segment() without NETIF_F_SG. This avoids
the need for desc_cnt_est in tg3_tso_bug() as in previous versions of this
patch series. Since Michael had previously raised concerns about gso_segment
without SG, I ran some netperf throughput tests. I used a small patch to force
tg3_tso_bug() to be called even when it is not needed [1].
root@linux-y64m:~# perf stat -r10 -ad netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d send
* original tg3_tso_bug() (ie. without patch 4/4)
781±2 10^6bits/s
6.60 cycle/bit
* gso_segment() without SG (current series)
801.0±0.9 10^6bits/s
5.79 cycle/bit
* gso_segment() with SG (alternate patch 4/4 [2])
783±2 10^6bits/s
7.25 cycle/bit
(For reference, with the original tg3_tso_bug() implementation but without
forcing it to be called, the throughput I get is 822±1 10^6bits/s @ 3.82
cycle/bit with 0 invocations of tg3_tso_bug)
[1] fault injection patch
---
drivers/net/ethernet/broadcom/tg3.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index cb77ae9..f9144dc 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -47,6 +47,7 @@
#include <linux/ssb/ssb_driver_gige.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
+#include <linux/debugfs.h>
#include <net/checksum.h>
#include <net/ip.h>
@@ -468,6 +469,27 @@ static const struct {
#define TG3_NUM_TEST ARRAY_SIZE(ethtool_test_keys)
+/* debugging stuff */
+static u32 tg3_do_mangle;
+static struct dentry *tg3_mangle_debugfs;
+
+static int __init tg3_mod_init(void)
+{
+ tg3_mangle_debugfs = debugfs_create_u32("tg3_do_mangle", S_IRUGO |
+ S_IWUSR, NULL,
+ &tg3_do_mangle);
+
+ return 0;
+}
+module_init(tg3_mod_init);
+
+static void __exit tg3_mod_exit(void)
+{
+ debugfs_remove(tg3_mangle_debugfs);
+}
+module_exit(tg3_mod_exit);
+/* --- */
+
static void tg3_write32(struct tg3 *tp, u32 off, u32 val)
{
writel(val, tp->regs + off);
@@ -8048,6 +8070,11 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
would_hit_hwbug = 1;
break;
}
+
+ if (tg3_do_mangle > 0) {
+ would_hit_hwbug = 4;
+ break;
+ }
}
}
--
[2] alternate patch 4
call gso_segment with SG (without removing it, actually)
---
drivers/net/ethernet/broadcom/tg3.c | 80 +++++++++++++++++++++++++++----------
1 file changed, 59 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index ee93b51..1ecb393 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -205,6 +205,9 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS flag, unsigned long *bits)
/* minimum number of free TX descriptors required to wake up TX process */
#define TG3_TX_WAKEUP_THRESH(tnapi) max_t(u32, (tnapi)->tx_pending / 4, \
MAX_SKB_FRAGS + 1)
+/* estimate a certain number of descriptors per gso segment */
+#define TG3_TX_DESC_PER_SEG(seg_nb) ((seg_nb) * 3)
+
#define TG3_TX_BD_DMA_MAX_2K 2048
#define TG3_TX_BD_DMA_MAX_4K 4096
@@ -7852,6 +7855,8 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi *tnapi,
}
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *, struct net_device *,
+ u32);
/* Returns true if the queue has been stopped. Note that it may have been
* restarted since.
@@ -7888,27 +7893,56 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
- struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)->gso_segs;
+ u32 desc_cnt_est = TG3_TX_DESC_PER_SEG(segs_remaining);
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
- if (netif_tx_queue_stopped(txq))
- return NETDEV_TX_BUSY;
+ if (unlikely(tg3_tx_avail(tnapi) <= desc_cnt_est)) {
+ if (!skb_is_nonlinear(skb) || skb_linearize(skb))
+ goto tg3_tso_bug_drop;
+ tg3_start_xmit(skb, tp->dev);
+ } else {
+ struct sk_buff *segs, *nskb;
- segs = skb_gso_segment(skb, tp->dev->features &
- ~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
- goto tg3_tso_bug_end;
+ segs = skb_gso_segment(skb, tp->dev->features &
+ ~(NETIF_F_TSO | NETIF_F_TSO6));
+ if (IS_ERR(segs) || !segs)
+ goto tg3_tso_bug_drop;
- do {
- nskb = segs;
- segs = segs->next;
- nskb->next = NULL;
- tg3_start_xmit(nskb, tp->dev);
- } while (segs);
+ do {
+ unsigned int desc_cnt = skb_shinfo(segs)->nr_frags + 1;
+
+ nskb = segs;
+ segs = segs->next;
+ nskb->next = NULL;
+
+ if (tg3_tx_avail(tnapi) <= segs_remaining - 1 +
+ desc_cnt && skb_linearize(nskb)) {
+ nskb->next = segs;
+ segs = nskb;
+ do {
+ nskb = segs->next;
+
+ dev_kfree_skb_any(segs);
+ segs = nskb;
+ } while (segs);
+ tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi));
+
+ goto tg3_tso_bug_drop;
+ }
+ if (--segs_remaining)
+ __tg3_start_xmit(nskb, tp->dev, segs_remaining);
+ else
+ tg3_start_xmit(nskb, tp->dev);
+ } while (segs);
-tg3_tso_bug_end:
+ dev_kfree_skb_any(skb);
+ }
+
+ return NETDEV_TX_OK;
+
+tg3_tso_bug_drop:
+ tp->tx_dropped++;
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
@@ -7917,6 +7951,12 @@ tg3_tso_bug_end:
/* hard_start_xmit for all devices */
static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ return __tg3_start_xmit(skb, dev, MAX_SKB_FRAGS + 1);
+}
+
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
+ struct net_device *dev, u32 stop_thresh)
+{
struct tg3 *tp = netdev_priv(dev);
u32 len, entry, base_flags, mss, vlan = 0;
u32 budget;
@@ -8129,7 +8169,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
tw32_tx_mbox(tnapi->prodmbox, entry);
tnapi->tx_prod = entry;
- tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ tg3_maybe_stop_txq(tnapi, txq, stop_thresh,
TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
@@ -12363,9 +12403,7 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
if ((ering->rx_pending > tp->rx_std_ring_mask) ||
(ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
(ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
- (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
- (tg3_flag(tp, TSO_BUG) &&
- (ering->tx_pending <= (MAX_SKB_FRAGS * 3))))
+ (ering->tx_pending <= MAX_SKB_FRAGS + 1))
return -EINVAL;
if (netif_running(dev)) {
--
^ permalink raw reply related
* Re: [PATCH net] cnic : Cleanup CONFIG_IPV6 & VLAN check
From: Michael Chan @ 2014-09-05 1:18 UTC (permalink / raw)
To: Anish Bhatt; +Cc: netdev, davem, jitendra.kalsaria, eddie.wai, randy.dunlap
In-Reply-To: <1409875977-31584-1-git-send-email-anish@chelsio.com>
On Thu, 2014-09-04 at 17:12 -0700, Anish Bhatt wrote:
> The cnic module needs to ensure that if ipv6 support is compiled as a module,
> then the cnic module cannot be compiled as built-in as it depends on ipv6.
> Made this check cleaner via Kconfig
>
> Use simpler IS_ENABLED for CONFIG_VLAN_8021Q check
>
> Signed-off-by: Anish Bhatt <anish@chelsio.com>
Acked-by: Michael Chan <mchan@broadcom.com>
>
> diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
> index 7dcfb19..d8d07a8 100644
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig
> @@ -84,7 +84,7 @@ config BNX2
>
> config CNIC
> tristate "QLogic CNIC support"
> - depends on PCI
> + depends on PCI && (IPV6 || IPV6=n)
> select BNX2
> select UIO
> ---help---
> diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
> index 27861a6..a6a9f28 100644
> --- a/drivers/net/ethernet/broadcom/cnic.c
> +++ b/drivers/net/ethernet/broadcom/cnic.c
> @@ -31,7 +31,7 @@
> #include <linux/if_vlan.h>
> #include <linux/prefetch.h>
> #include <linux/random.h>
> -#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
> +#if IS_ENABLED(CONFIG_VLAN_8021Q)
> #define BCM_VLAN 1
> #endif
> #include <net/ip.h>
> @@ -3685,7 +3685,7 @@ static int cnic_get_v4_route(struct sockaddr_in *dst_addr,
> static int cnic_get_v6_route(struct sockaddr_in6 *dst_addr,
> struct dst_entry **dst)
> {
> -#if defined(CONFIG_IPV6) || (defined(CONFIG_IPV6_MODULE) && defined(MODULE))
> +#if IS_ENABLED(CONFIG_IPV6)
> struct flowi6 fl6;
>
> memset(&fl6, 0, sizeof(fl6));
^ permalink raw reply
* Re: [RFC PATCH 1/3] net: Add function for parsing the header length out of linear ethernet frames
From: Alexander Duyck @ 2014-09-05 1:00 UTC (permalink / raw)
To: Eric Dumazet, Alexander Duyck; +Cc: netdev, davem
In-Reply-To: <1409873788.26422.130.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/04/2014 04:36 PM, Eric Dumazet wrote:
> On Thu, 2014-09-04 at 19:13 -0400, Alexander Duyck wrote:
>> This patch updates some of the flow_dissector api so that it can be used to
>> parse the length of ethernet buffers stored in fragments. Most of the
>> changes needed were to __skb_get_poff as it needed to be updated to support
>> sending a linear buffer instead of a skb.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>> include/linux/etherdevice.h | 1 +
>> include/linux/skbuff.h | 2 ++
>> include/net/flow_keys.h | 2 ++
>> net/core/flow_dissector.c | 41 ++++++++++++++++++++++++++---------------
>> net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
>> 5 files changed, 58 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> index 12f48ca..f55667e 100644
>> --- a/net/core/flow_dissector.c
>> +++ b/net/core/flow_dissector.c
>> @@ -13,6 +13,7 @@
>> #include <linux/if_pppox.h>
>> #include <linux/ppp_defs.h>
>> #include <net/flow_keys.h>
>> +#include <scsi/fc/fc_fcoe.h>
>>
>> /* copy saddr & daddr, possibly using 64bit load/store
>> * Equivalent to : flow->src = iph->saddr;
>> @@ -118,7 +119,7 @@ ipv6:
>> nhoff += sizeof(struct ipv6hdr);
>>
>> flow_label = ip6_flowlabel(iph);
>> - if (flow_label) {
>> + if (flow_label && skb) {
> Hmpff... undocumented bit here....
I'll add a comment. It is basically just forcing it to continue parsing
for the case where skb is NULL which currently only applies to
eth_get_headlen.
>
> @@ -369,6 +365,21 @@ u32 __skb_get_poff(const struct sk_buff *skb)
> return poff;
> }
>
> +/* __skb_get_poff() returns the offset to the payload as far as it could
> + * be dissected. The main user is currently BPF, so that we can dynamically
> + * truncate packets without needing to push actual payload to the user
> + * space and can analyze headers only, instead.
> + */
> +u32 __skb_get_poff(const struct sk_buff *skb)
> +{
> + struct flow_keys keys;
> +
> + if (!skb_flow_dissect(skb, &keys))
> + return 0;
> +
> + return ___skb_get_poff(skb, skb->data, &keys, skb->len);
> hlen is not skb->len, but skb_headlen(skb)
My bad. I will fix that for the next one.
>> +}
>> +
>> static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
>> {
>> #ifdef CONFIG_XPS
>> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>> index 5cebca1..f0299ac 100644
>> --- a/net/ethernet/eth.c
>> +++ b/net/ethernet/eth.c
>> @@ -146,6 +146,33 @@ int eth_rebuild_header(struct sk_buff *skb)
>> EXPORT_SYMBOL(eth_rebuild_header);
>>
>> /**
>> + * eth_get_headlen - determine the the length of header for an ethernet frame
>> + * @data: pointer to start of frame
>> + * @len: total length of frame
>> + *
>> + * Make a best effort attempt to pull the length for all of the headers for
>> + * a given frame in a linear buffer.
>> + */
>> +u32 eth_get_headlen(void *data, unsigned int len)
>> +{
>> + const struct ethhdr *eth = (const struct ethhdr *)data;
>> + struct flow_keys keys;
>> +
>> + /* this should never happen, but better safe than sorry */
>> + if (len < sizeof(*eth))
>> + return len;
>> +
>> + /* parse any remaining L2/L3 headers, check for L4 */
>> + if (!__skb_flow_dissect(NULL, &keys, data,
>> + eth->h_proto, sizeof(*eth), len))
>> + return max_t(u32, keys.thoff, sizeof(*eth));
> Not sure keys.thoff is valid at this point ?
>
It should be 0 if it didn't find any value. __skb_flow_dissect starts
by doing a memset 0 on the keys. So I am using the max_t to force it to
at least include the Ethernet header in the length to pull as otherwise
we will trigger errors on eth_type_trans.
Thanks,
Alex
^ permalink raw reply
* RE: [PATCH net] cnic : Cleanup CONFIG_IPV6 & VLAN check
From: Anish Bhatt @ 2014-09-05 0:35 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, mchan@broadcom.com, eddie.wai@broadcom.com,
ameen.rahman@qlogic.com
In-Reply-To: <1409875977-31584-1-git-send-email-anish@chelsio.com>
Most of the people identified by get_maintainers.pl don't seem to be at the
script identified email addresses, modifying the cc list accordingly.
^ permalink raw reply
* [PATCH net] cnic : Cleanup CONFIG_IPV6 & VLAN check
From: Anish Bhatt @ 2014-09-05 0:12 UTC (permalink / raw)
To: netdev
Cc: davem, jitendra.kalsaria, mchan, eddie.wai, randy.dunlap,
Anish Bhatt
The cnic module needs to ensure that if ipv6 support is compiled as a module,
then the cnic module cannot be compiled as built-in as it depends on ipv6.
Made this check cleaner via Kconfig
Use simpler IS_ENABLED for CONFIG_VLAN_8021Q check
Signed-off-by: Anish Bhatt <anish@chelsio.com>
diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 7dcfb19..d8d07a8 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -84,7 +84,7 @@ config BNX2
config CNIC
tristate "QLogic CNIC support"
- depends on PCI
+ depends on PCI && (IPV6 || IPV6=n)
select BNX2
select UIO
---help---
diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index 27861a6..a6a9f28 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -31,7 +31,7 @@
#include <linux/if_vlan.h>
#include <linux/prefetch.h>
#include <linux/random.h>
-#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+#if IS_ENABLED(CONFIG_VLAN_8021Q)
#define BCM_VLAN 1
#endif
#include <net/ip.h>
@@ -3685,7 +3685,7 @@ static int cnic_get_v4_route(struct sockaddr_in *dst_addr,
static int cnic_get_v6_route(struct sockaddr_in6 *dst_addr,
struct dst_entry **dst)
{
-#if defined(CONFIG_IPV6) || (defined(CONFIG_IPV6_MODULE) && defined(MODULE))
+#if IS_ENABLED(CONFIG_IPV6)
struct flowi6 fl6;
memset(&fl6, 0, sizeof(fl6));
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-05 0:10 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <54086645.3010102@redhat.com>
On Thu, Sep 4, 2014 at 6:16 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
> On 03/09/14 23:47, Mahesh Bandewar wrote:
>>
>> Earlier change to use usable slave array for TLB mode had an additional
>> performance advantage. So extending the same logic to all other modes
>> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
>> Also consolidating this with the earlier TLB change.
>>
>> The main idea is to build the usable slaves array in the control path
>> and use that array for slave selection during xmit operation.
>>
>> Measured performance in a setup with a bond of 4x1G NICs with 200
>> instances of netperf for the modes involved (3ad, xor, tlb)
>> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>>
>> Mode TPS-Before TPS-After
>>
>> 802.3ad : 468,694 493,101
>> TLB (lb=0): 392,583 392,965
>> XOR : 475,696 484,517
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>
> <<snip>>
> Hi Mahesh,
> I really like this idea, but I think this patch is far from ready and needs
> more thorough examination.
>
I see that you have looked it from the XOR-mode perspective and thanks
for that. I admit that, I did not test that mode thoroughly.
>
>> diff --git a/drivers/net/bonding/bond_main.c
>> b/drivers/net/bonding/bond_main.c
>> index f0f5eab0fab1..b0e8e7cfa10f 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>
>
> What happens to modes that don't have miimon enabled i.e. if I do: modprobe
> bonding mode=2 and enslave something then the slave array will never get
> created and I won't be able to xmit anything ever because miimon is 0 by
> default and even if it wasn't I'll have to wait for it to create the slave
> array before I can do any transmissions.
> By the way, I just noticed that arp_interval is actually supported in XOR
> mode.
>
Yes, this should not be restricted to just the miimon case, and it
should be irrespective of miimon, arp-mon enabled / disabled etc. I'll
correct that!
>
>> @@ -1693,6 +1693,9 @@ static int __bond_release_one(struct net_device
>> *bond_dev,
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_unbind_slave(slave);
>>
> ^^^^^^^^^^^^^^^^^^^^^^^
> Empty line.
>
>
>> + else if (BOND_MODE(bond) == BOND_MODE_XOR)
>> + bond_update_slave_arr(bond, slave);
>> +
>> write_unlock_bh(&bond->lock);
>>
>> netdev_info(bond_dev, "Releasing %s interface %s\n",
>> @@ -2009,6 +2012,9 @@ static void bond_miimon_commit(struct bonding *bond)
>> bond_alb_handle_link_change(bond, slave,
>> BOND_LINK_UP);
>>
>> + if (BOND_MODE(bond) == BOND_MODE_XOR)
>> + bond_update_slave_arr(bond, NULL);
>> +
>> if (!bond->curr_active_slave ||
>> (slave == bond->primary_slave))
>> goto do_failover;
>> @@ -2037,6 +2043,9 @@ static void bond_miimon_commit(struct bonding *bond)
>> bond_alb_handle_link_change(bond, slave,
>>
>> BOND_LINK_DOWN);
>>
>> + if (BOND_MODE(bond) == BOND_MODE_XOR)
>> + bond_update_slave_arr(bond, slave);
>> +
>> if (slave ==
>> rcu_access_pointer(bond->curr_active_slave))
>> goto do_failover;
>>
>> @@ -3149,6 +3158,7 @@ static int bond_open(struct net_device *bond_dev)
>> static int bond_close(struct net_device *bond_dev)
>> {
>> struct bonding *bond = netdev_priv(bond_dev);
>> + struct bond_up_slave *arr;
>>
>> bond_work_cancel_all(bond);
>> bond->send_peer_notif = 0;
>> @@ -3156,6 +3166,10 @@ static int bond_close(struct net_device *bond_dev)
>> bond_alb_deinitialize(bond);
>> bond->recv_probe = NULL;
>>
>> + arr = rtnl_dereference(bond->slave_arr);
>> + if (arr)
>> + kfree_rcu(arr, rcu);
>> +
>
> ^^^^^^^^^^^^^^
> Bond close is dealt with, but what happens after: ip l set bond down, ip l
> set bond up
> or alternatively: ip l set bond down, echo -slave > slaves
>
> The array is freed but slave_arr still points to it.
>
good point. I'll update it. Also applicable to the earlier TLB mode,
so will add that into the patch for the stable.
>
>> return 0;
>> }
>>
>> @@ -3684,15 +3698,84 @@ static int bond_xmit_activebackup(struct sk_buff
>> *skb, struct net_device *bond_d
>> return NETDEV_TX_OK;
>> }
>>
>> -/* In bond_xmit_xor() , we determine the output device by using a pre-
>> - * determined xmit_hash_policy(), If the selected device is not enabled,
>> - * find the next active slave.
>> +/* Build the usable slaves array in control path for modes that use
>> xmit-hash
>> + * to determine the slave interface -
>> + * (a) BOND_MODE_8023AD
>> + * (b) BOND_MODE_XOR
>> + * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>> */
>> -static int bond_xmit_xor(struct sk_buff *skb, struct net_device
>> *bond_dev)
>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave)
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> In general any failing in this function means that the old array stays with
> the old slave list. I think that you really should revisit the places that
> use this after the patch and see if that won't cause any problems.
>
The only really problematic scenario is where one of the slaves is
disappearing (as pointed out by Jay earlier) and I'll fix that in the
next version. For rest of the cases, the worse that could happen is
the slave that is selected wont be able to transmit that packet. As I
have explained earlier, in case of failure to allocate such small
portion of memory (less than 1k in worse scenario!) machine has lot
other problems to handle than not being able to send a packet out.
>
>> {
>> - struct bonding *bond = netdev_priv(bond_dev);
>> + struct slave *slave;
>> + struct list_head *iter;
>> + struct bond_up_slave *new_arr, *old_arr;
>> + int slaves_in_agg;
>> + int agg_id = 0;
>> + int ret = 0;
>> +
>> + new_arr = kzalloc(offsetof(struct bond_up_slave,
>> arr[bond->slave_cnt]),
>> + GFP_ATOMIC);
>> + if (!new_arr) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> + struct ad_info ad_info;
>> +
>> + if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Called with RCU otherwise sparse won't be happy. Perhaps
> bond_3ad_get_active_agg_info() ?
>
Yes, corrected!
>
>> + pr_debug("__bond_3ad_get_active_agg_info
>> failed\n");
>> + kfree_rcu(new_arr, rcu);
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + slaves_in_agg = ad_info.ports;
>> + agg_id = ad_info.aggregator_id;
>> + }
>> + bond_for_each_slave(bond, slave, iter) {
>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> + struct aggregator *agg;
>> +
>> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> + if (!agg || agg->aggregator_identifier != agg_id)
>> + continue;
>> + }
>> + if (!bond_slave_can_tx(slave))
>> + continue;
>> + if (skipslave == slave)
>> + continue;
>> + new_arr->arr[new_arr->count++] = slave;
>> + }
>>
>> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) %
>> bond->slave_cnt);
>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>> + lockdep_rtnl_is_held() ||
>> + lockdep_is_held(&bond->lock)
>> ||
>> +
>> lockdep_is_held(&bond->curr_slave_lock));
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This line is the most troublesome for me, which lock is it ? Does this mean
> that whichever I hold from the three I can update the slave array ?
> I don't think this is worked out well, you should explicitly specify how and
> why it is safe to update this under each of the locks and maybe you'll be
> able to reduce the lock list :-)
>
This is primarily because of different code paths it's taking to reach
here. In all these cases, one of those locks is held. Unfortunately
there are three such locks that I have identified (for all three
modes involved) and hence the above line.
>
>> + rcu_assign_pointer(bond->slave_arr, new_arr);
>> + if (old_arr)
>> + kfree_rcu(old_arr, rcu);
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +/* Use this Xmit function for 3AD as well as XOR modes. The current
>> + * usable slave array is formed in the control path. The xmit function
>> + * just calculates hash and sends the packet out.
>> + */
>> +int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct bonding *bond = netdev_priv(dev);
>> + struct slave *slave;
>> + struct bond_up_slave *slaves;
>> +
>> + slaves = rcu_dereference(bond->slave_arr);
>> + if (slaves && slaves->count) {
>> + slave = slaves->arr[bond_xmit_hash(bond, skb) %
>> slaves->count];
>> + bond_dev_queue_xmit(bond, skb, slave->dev);
>> + } else {
>> + dev_kfree_skb_any(skb);
>> + atomic_long_inc(&dev->tx_dropped);
>> + }
>>
>> return NETDEV_TX_OK;
>> }
>> @@ -3794,12 +3877,11 @@ static netdev_tx_t __bond_start_xmit(struct
>> sk_buff *skb, struct net_device *dev
>> return bond_xmit_roundrobin(skb, dev);
>> case BOND_MODE_ACTIVEBACKUP:
>> return bond_xmit_activebackup(skb, dev);
>> + case BOND_MODE_8023AD:
>> case BOND_MODE_XOR:
>> - return bond_xmit_xor(skb, dev);
>> + return bond_3ad_xor_xmit(skb, dev);
>> case BOND_MODE_BROADCAST:
>> return bond_xmit_broadcast(skb, dev);
>> - case BOND_MODE_8023AD:
>> - return bond_3ad_xmit_xor(skb, dev);
>> case BOND_MODE_ALB:
>> return bond_alb_xmit(skb, dev);
>> case BOND_MODE_TLB:
>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>> index aace510d08d1..4a6195c0de60 100644
>> --- a/drivers/net/bonding/bonding.h
>> +++ b/drivers/net/bonding/bonding.h
>> @@ -177,6 +177,12 @@ struct slave {
>> struct kobject kobj;
>> };
>>
>> +struct bond_up_slave {
>> + unsigned int count;
>> + struct rcu_head rcu;
>> + struct slave *arr[0];
>> +};
>> +
>> /*
>> * Link pseudo-state only used internally by monitors
>> */
>> @@ -196,6 +202,7 @@ struct bonding {
>> struct slave __rcu *curr_active_slave;
>> struct slave __rcu *current_arp_slave;
>> struct slave *primary_slave;
>> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves
>> */
>> bool force_primary;
>> s32 slave_cnt; /* never change this value outside the
>> attach/detach wrappers */
>> int (*recv_probe)(const struct sk_buff *, struct bonding *,
>> @@ -527,6 +534,7 @@ const char *bond_slave_link_status(s8 link);
>> struct bond_vlan_tag *bond_verify_device_path(struct net_device
>> *start_dev,
>> struct net_device *end_dev,
>> int level);
>> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
>>
>> #ifdef CONFIG_PROC_FS
>> void bond_create_proc_entry(struct bonding *bond);
>>
>
^ permalink raw reply
* Re: [PATCH net] core: Untag packets after rx_handler has run.
From: Vlad Yasevich @ 2014-09-04 23:48 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Vlad Yasevich, Jiri Pirko, netdev@vger.kernel.org,
Florian Zumbiehl, Eric Dumazet, Matthew Rosato
In-Reply-To: <CAADnVQJa=a9NP68njLBwyDLB-FaXYeUaCZ+ZGROZsOQeTsEvVg@mail.gmail.com>
On 09/04/2014 05:54 PM, Alexei Starovoitov wrote:
> On Thu, Sep 4, 2014 at 2:01 PM, Vlad Yasevich <vyasevic@redhat.com> wrote:
>> On 09/04/2014 04:43 PM, Alexei Starovoitov wrote:
>>> On Thu, Sep 04, 2014 at 03:29:00PM -0400, Vlad Yasevich wrote:
>>>>> nack. This will definitelly break several stacked setups.
>>>>
>>>> Which ones? The only thing I can see that would behave differently
>>>> is something like:
>>>>
>>>> vlan0 bridge0
>>>> | |
>>>> +-------- eth0
>>>>
>>>> In this case, the old code would give an untagged packet to the bridge
>>>> and the new code would give a tagged packet.
>>>>
>>>> This set-up is a bit ambiguous. Remove the vlan, and bridge gets a tagged
>>>> traffic even though the vlan has no relationship to the bridge.
>>>>
>>>> I've tested a couple of different stacked setups and they all seem to work.
>>>
>>> 2nd nack.
>>> It will break user space, including our setup that has:
>>> vlanX OVS
>>> | |
>>> +------ eth0
>>>
>>> vlan device has IP assigned and all tagged traffic goes through the stack
>>> and into control plane process. ovs datapath keeps managing eth0 with
>>> all other vlans.
>>>
>>
>> Did you specially configure OVS to pass the traffic up the stack? I see
>> OVS will only pass LOOPBACK packets. All others it seems to consume.
>>
>> Can the same be accomplished with a tagged internal port?
>
> our ovs config is not using internal port. vlan device is used as
> control interface and should be independent of ovs datapath.
> Theoretically it may be possible to use ovs for both, but very dangerous,
> when control and data are going through the same datapath.
> Any ovs programming mistake will kill control plane and whole
> hypervisor will become inaccessible.
>
>> The reason I am asking, is I am trying to figure out if this is
>> a valid config. It seems very hard to get right and seems to work almost
>> by accident at times. For example, in the bridge scenario I described.
>> vlan and bridge have to share a mac address for that work.
>
> I think it's not valid vs invalid config.
> this was the behavior of vlan devices for long time. vlan was parsed
> and send to vlan_dev _before_ rx_handler. I suspect there is more
> than one user app that is relying on that.
> I can change our stuff to do something different, but I think we
> should not be breaking vlan behavior for others.
>
I see. So vlan device always appears to take precedence over the rx_handler
if they are at the same level and we can't break this.
OK, this means that to solve this we have to expose the vlan filtering
API on macvtap devices as well.
Thanks
-vlad
^ permalink raw reply
* Re: Regression: TCP connections fail over wireless: bad cksum?
From: Eric Dumazet @ 2014-09-04 23:38 UTC (permalink / raw)
To: Ted Percival; +Cc: netdev
In-Reply-To: <1409873287.26422.127.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, 2014-09-04 at 16:28 -0700, Eric Dumazet wrote:
> tc -s -d qd show dev wlan0
>
> tc qdisc replace dev wlan0 fq
I forgot the root keyword :
tc qdisc replace dev wlan0 root fq
^ permalink raw reply
* Re: [RFC PATCH 1/3] net: Add function for parsing the header length out of linear ethernet frames
From: Eric Dumazet @ 2014-09-04 23:36 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem
In-Reply-To: <20140904231333.12376.9881.stgit@ahduyck-bv4.jf.intel.com>
On Thu, 2014-09-04 at 19:13 -0400, Alexander Duyck wrote:
> This patch updates some of the flow_dissector api so that it can be used to
> parse the length of ethernet buffers stored in fragments. Most of the
> changes needed were to __skb_get_poff as it needed to be updated to support
> sending a linear buffer instead of a skb.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> include/linux/etherdevice.h | 1 +
> include/linux/skbuff.h | 2 ++
> include/net/flow_keys.h | 2 ++
> net/core/flow_dissector.c | 41 ++++++++++++++++++++++++++---------------
> net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
> 5 files changed, 58 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 9c5529d..733980f 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -29,6 +29,7 @@
> #include <asm/bitsperlong.h>
>
> #ifdef __KERNEL__
> +u32 eth_get_headlen(void *data, unsigned int max_len);
> __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
> extern const struct header_ops eth_header_ops;
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 02529fc..77d4d6f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3217,6 +3217,8 @@ bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
> int skb_checksum_setup(struct sk_buff *skb, bool recalculate);
>
> u32 __skb_get_poff(const struct sk_buff *skb);
> +u32 ___skb_get_poff(const struct sk_buff *skb, void *data,
> + const struct flow_keys *keys, int hlen);
>
> /**
> * skb_head_is_locked - Determine if the skb->head is locked down
> diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
> index 9a03f73..7ee2df0 100644
> --- a/include/net/flow_keys.h
> +++ b/include/net/flow_keys.h
> @@ -40,4 +40,6 @@ static inline __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8
> return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
> }
> u32 flow_hash_from_keys(struct flow_keys *keys);
> +unsigned int flow_get_hlen(const unsigned char *data, unsigned int max_len,
> + __be16 protocol);
> #endif
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 12f48ca..f55667e 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -13,6 +13,7 @@
> #include <linux/if_pppox.h>
> #include <linux/ppp_defs.h>
> #include <net/flow_keys.h>
> +#include <scsi/fc/fc_fcoe.h>
>
> /* copy saddr & daddr, possibly using 64bit load/store
> * Equivalent to : flow->src = iph->saddr;
> @@ -118,7 +119,7 @@ ipv6:
> nhoff += sizeof(struct ipv6hdr);
>
> flow_label = ip6_flowlabel(iph);
> - if (flow_label) {
> + if (flow_label && skb) {
Hmpff... undocumented bit here....
> /* Awesome, IPv6 packet has a flow label so we can
> * use that to represent the ports without any
> * further dissection.
> @@ -165,6 +166,9 @@ ipv6:
> return false;
> }
> }
> + case htons(ETH_P_FCOE):
> + flow->thoff = (u16)(nhoff + FCOE_HEADER_LEN);
> + /* fall through */
> default:
> return false;
> }
> @@ -316,26 +320,18 @@ u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
> }
> EXPORT_SYMBOL(__skb_tx_hash);
>
> -/* __skb_get_poff() returns the offset to the payload as far as it could
> - * be dissected. The main user is currently BPF, so that we can dynamically
> - * truncate packets without needing to push actual payload to the user
> - * space and can analyze headers only, instead.
> - */
> -u32 __skb_get_poff(const struct sk_buff *skb)
> +u32 ___skb_get_poff(const struct sk_buff *skb, void *data,
> + const struct flow_keys *keys, int hlen)
> {
> - struct flow_keys keys;
> - u32 poff = 0;
> -
> - if (!skb_flow_dissect(skb, &keys))
> - return 0;
> + u32 poff = keys->thoff;
>
> - poff += keys.thoff;
> - switch (keys.ip_proto) {
> + switch (keys->ip_proto) {
> case IPPROTO_TCP: {
> const struct tcphdr *tcph;
> struct tcphdr _tcph;
>
> - tcph = skb_header_pointer(skb, poff, sizeof(_tcph), &_tcph);
> + tcph = __skb_header_pointer(skb, poff, sizeof(_tcph),
> + data, hlen, &_tcph);
> if (!tcph)
> return poff;
>
> @@ -369,6 +365,21 @@ u32 __skb_get_poff(const struct sk_buff *skb)
> return poff;
> }
>
> +/* __skb_get_poff() returns the offset to the payload as far as it could
> + * be dissected. The main user is currently BPF, so that we can dynamically
> + * truncate packets without needing to push actual payload to the user
> + * space and can analyze headers only, instead.
> + */
> +u32 __skb_get_poff(const struct sk_buff *skb)
> +{
> + struct flow_keys keys;
> +
> + if (!skb_flow_dissect(skb, &keys))
> + return 0;
> +
> + return ___skb_get_poff(skb, skb->data, &keys, skb->len);
hlen is not skb->len, but skb_headlen(skb)
> +}
> +
> static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
> {
> #ifdef CONFIG_XPS
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 5cebca1..f0299ac 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -146,6 +146,33 @@ int eth_rebuild_header(struct sk_buff *skb)
> EXPORT_SYMBOL(eth_rebuild_header);
>
> /**
> + * eth_get_headlen - determine the the length of header for an ethernet frame
> + * @data: pointer to start of frame
> + * @len: total length of frame
> + *
> + * Make a best effort attempt to pull the length for all of the headers for
> + * a given frame in a linear buffer.
> + */
> +u32 eth_get_headlen(void *data, unsigned int len)
> +{
> + const struct ethhdr *eth = (const struct ethhdr *)data;
> + struct flow_keys keys;
> +
> + /* this should never happen, but better safe than sorry */
> + if (len < sizeof(*eth))
> + return len;
> +
> + /* parse any remaining L2/L3 headers, check for L4 */
> + if (!__skb_flow_dissect(NULL, &keys, data,
> + eth->h_proto, sizeof(*eth), len))
> + return max_t(u32, keys.thoff, sizeof(*eth));
Not sure keys.thoff is valid at this point ?
> +
> + /* parse for any L4 headers */
> + return min_t(u32, ___skb_get_poff(NULL, data, &keys, len), len);
> +}
> +EXPORT_SYMBOL(eth_get_headlen);
> +
> +/**
> * eth_type_trans - determine the packet's protocol ID.
> * @skb: received socket data
> * @dev: receiving network device
>
^ permalink raw reply
* Re: Regression: TCP connections fail over wireless: bad cksum?
From: Eric Dumazet @ 2014-09-04 23:28 UTC (permalink / raw)
To: Ted Percival; +Cc: netdev
In-Reply-To: <5408F3FB.2010107@tedp.id.au>
On Thu, 2014-09-04 at 17:21 -0600, Ted Percival wrote:
> # ethtool -k wlan0
> Features for wlan0:
> rx-checksumming: off [fixed]
> tx-checksumming: off
> tx-checksum-ipv4: off [fixed]
> tx-checksum-ip-generic: off [fixed]
> tx-checksum-ipv6: off [fixed]
> tx-checksum-fcoe-crc: off [fixed]
> tx-checksum-sctp: off [fixed]
> scatter-gather: off
> tx-scatter-gather: off [fixed]
> tx-scatter-gather-fraglist: off [fixed]
> tcp-segmentation-offload: off
> tx-tcp-segmentation: off [fixed]
> tx-tcp-ecn-segmentation: off [fixed]
> tx-tcp6-segmentation: off [fixed]
> udp-fragmentation-offload: off [fixed]
> generic-segmentation-offload: off [requested on]
> generic-receive-offload: on
> large-receive-offload: off [fixed]
> rx-vlan-offload: off [fixed]
> tx-vlan-offload: off [fixed]
> ntuple-filters: off [fixed]
> receive-hashing: off [fixed]
> highdma: off [fixed]
> rx-vlan-filter: off [fixed]
> vlan-challenged: off [fixed]
> tx-lockless: off [fixed]
> netns-local: on [fixed]
> tx-gso-robust: off [fixed]
> tx-fcoe-segmentation: off [fixed]
> tx-gre-segmentation: off [fixed]
> tx-ipip-segmentation: off [fixed]
> tx-sit-segmentation: off [fixed]
> tx-udp_tnl-segmentation: off [fixed]
> tx-mpls-segmentation: off [fixed]
> fcoe-mtu: off [fixed]
> tx-nocache-copy: off
> loopback: off [fixed]
> rx-fcs: off [fixed]
> rx-all: off [fixed]
> tx-vlan-stag-hw-insert: off [fixed]
> rx-vlan-stag-hw-parse: off [fixed]
> rx-vlan-stag-filter: off [fixed]
> l2-fwd-offload: off [fixed]
> busy-poll: off [fixed]
>
> # ethtool -K wlan0 tx off
> Cannot change tx-checksumming
>
> I will try to isolate the commit that caused the regression.
Ah well, I guess we know it ;)
tc -s -d qd show dev wlan0
tc qdisc replace dev wlan0 fq
This should temporarily help.
^ permalink raw reply
* [RFC PATCH 1/3] net: Add function for parsing the header length out of linear ethernet frames
From: Alexander Duyck @ 2014-09-04 23:13 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet
In-Reply-To: <20140904230958.12376.2845.stgit@ahduyck-bv4.jf.intel.com>
This patch updates some of the flow_dissector api so that it can be used to
parse the length of ethernet buffers stored in fragments. Most of the
changes needed were to __skb_get_poff as it needed to be updated to support
sending a linear buffer instead of a skb.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
include/linux/etherdevice.h | 1 +
include/linux/skbuff.h | 2 ++
include/net/flow_keys.h | 2 ++
net/core/flow_dissector.c | 41 ++++++++++++++++++++++++++---------------
net/ethernet/eth.c | 27 +++++++++++++++++++++++++++
5 files changed, 58 insertions(+), 15 deletions(-)
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 9c5529d..733980f 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -29,6 +29,7 @@
#include <asm/bitsperlong.h>
#ifdef __KERNEL__
+u32 eth_get_headlen(void *data, unsigned int max_len);
__be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev);
extern const struct header_ops eth_header_ops;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 02529fc..77d4d6f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3217,6 +3217,8 @@ bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
int skb_checksum_setup(struct sk_buff *skb, bool recalculate);
u32 __skb_get_poff(const struct sk_buff *skb);
+u32 ___skb_get_poff(const struct sk_buff *skb, void *data,
+ const struct flow_keys *keys, int hlen);
/**
* skb_head_is_locked - Determine if the skb->head is locked down
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 9a03f73..7ee2df0 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -40,4 +40,6 @@ static inline __be32 skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8
return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
}
u32 flow_hash_from_keys(struct flow_keys *keys);
+unsigned int flow_get_hlen(const unsigned char *data, unsigned int max_len,
+ __be16 protocol);
#endif
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 12f48ca..f55667e 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -13,6 +13,7 @@
#include <linux/if_pppox.h>
#include <linux/ppp_defs.h>
#include <net/flow_keys.h>
+#include <scsi/fc/fc_fcoe.h>
/* copy saddr & daddr, possibly using 64bit load/store
* Equivalent to : flow->src = iph->saddr;
@@ -118,7 +119,7 @@ ipv6:
nhoff += sizeof(struct ipv6hdr);
flow_label = ip6_flowlabel(iph);
- if (flow_label) {
+ if (flow_label && skb) {
/* Awesome, IPv6 packet has a flow label so we can
* use that to represent the ports without any
* further dissection.
@@ -165,6 +166,9 @@ ipv6:
return false;
}
}
+ case htons(ETH_P_FCOE):
+ flow->thoff = (u16)(nhoff + FCOE_HEADER_LEN);
+ /* fall through */
default:
return false;
}
@@ -316,26 +320,18 @@ u16 __skb_tx_hash(const struct net_device *dev, struct sk_buff *skb,
}
EXPORT_SYMBOL(__skb_tx_hash);
-/* __skb_get_poff() returns the offset to the payload as far as it could
- * be dissected. The main user is currently BPF, so that we can dynamically
- * truncate packets without needing to push actual payload to the user
- * space and can analyze headers only, instead.
- */
-u32 __skb_get_poff(const struct sk_buff *skb)
+u32 ___skb_get_poff(const struct sk_buff *skb, void *data,
+ const struct flow_keys *keys, int hlen)
{
- struct flow_keys keys;
- u32 poff = 0;
-
- if (!skb_flow_dissect(skb, &keys))
- return 0;
+ u32 poff = keys->thoff;
- poff += keys.thoff;
- switch (keys.ip_proto) {
+ switch (keys->ip_proto) {
case IPPROTO_TCP: {
const struct tcphdr *tcph;
struct tcphdr _tcph;
- tcph = skb_header_pointer(skb, poff, sizeof(_tcph), &_tcph);
+ tcph = __skb_header_pointer(skb, poff, sizeof(_tcph),
+ data, hlen, &_tcph);
if (!tcph)
return poff;
@@ -369,6 +365,21 @@ u32 __skb_get_poff(const struct sk_buff *skb)
return poff;
}
+/* __skb_get_poff() returns the offset to the payload as far as it could
+ * be dissected. The main user is currently BPF, so that we can dynamically
+ * truncate packets without needing to push actual payload to the user
+ * space and can analyze headers only, instead.
+ */
+u32 __skb_get_poff(const struct sk_buff *skb)
+{
+ struct flow_keys keys;
+
+ if (!skb_flow_dissect(skb, &keys))
+ return 0;
+
+ return ___skb_get_poff(skb, skb->data, &keys, skb->len);
+}
+
static inline int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
{
#ifdef CONFIG_XPS
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 5cebca1..f0299ac 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -146,6 +146,33 @@ int eth_rebuild_header(struct sk_buff *skb)
EXPORT_SYMBOL(eth_rebuild_header);
/**
+ * eth_get_headlen - determine the the length of header for an ethernet frame
+ * @data: pointer to start of frame
+ * @len: total length of frame
+ *
+ * Make a best effort attempt to pull the length for all of the headers for
+ * a given frame in a linear buffer.
+ */
+u32 eth_get_headlen(void *data, unsigned int len)
+{
+ const struct ethhdr *eth = (const struct ethhdr *)data;
+ struct flow_keys keys;
+
+ /* this should never happen, but better safe than sorry */
+ if (len < sizeof(*eth))
+ return len;
+
+ /* parse any remaining L2/L3 headers, check for L4 */
+ if (!__skb_flow_dissect(NULL, &keys, data,
+ eth->h_proto, sizeof(*eth), len))
+ return max_t(u32, keys.thoff, sizeof(*eth));
+
+ /* parse for any L4 headers */
+ return min_t(u32, ___skb_get_poff(NULL, data, &keys, len), len);
+}
+EXPORT_SYMBOL(eth_get_headlen);
+
+/**
* eth_type_trans - determine the packet's protocol ID.
* @skb: received socket data
* @dev: receiving network device
^ permalink raw reply related
* [RFC PATCH 3/3] ixgbe: use new eth_get_headlen interface
From: Alexander Duyck @ 2014-09-04 23:13 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet
In-Reply-To: <20140904230958.12376.2845.stgit@ahduyck-bv4.jf.intel.com>
Update ixgbe to drop the ixgbe_get_headlen function in favor of eth_get_headlen.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 116 -------------------------
1 file changed, 1 insertion(+), 115 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 53fbf06..09c3746 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1521,120 +1521,6 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)
ixgbe_release_rx_desc(rx_ring, i);
}
-/**
- * ixgbe_get_headlen - determine size of header for RSC/LRO/GRO/FCOE
- * @data: pointer to the start of the headers
- * @max_len: total length of section to find headers in
- *
- * This function is meant to determine the length of headers that will
- * be recognized by hardware for LRO, GRO, and RSC offloads. The main
- * motivation of doing this is to only perform one pull for IPv4 TCP
- * packets so that we can do basic things like calculating the gso_size
- * based on the average data per packet.
- **/
-static unsigned int ixgbe_get_headlen(unsigned char *data,
- unsigned int max_len)
-{
- union {
- unsigned char *network;
- /* l2 headers */
- struct ethhdr *eth;
- struct vlan_hdr *vlan;
- /* l3 headers */
- struct iphdr *ipv4;
- struct ipv6hdr *ipv6;
- } hdr;
- __be16 protocol;
- u8 nexthdr = 0; /* default to not TCP */
- u8 hlen;
-
- /* this should never happen, but better safe than sorry */
- if (max_len < ETH_HLEN)
- return max_len;
-
- /* initialize network frame pointer */
- hdr.network = data;
-
- /* set first protocol and move network header forward */
- protocol = hdr.eth->h_proto;
- hdr.network += ETH_HLEN;
-
- /* handle any vlan tag if present */
- if (protocol == htons(ETH_P_8021Q)) {
- if ((hdr.network - data) > (max_len - VLAN_HLEN))
- return max_len;
-
- protocol = hdr.vlan->h_vlan_encapsulated_proto;
- hdr.network += VLAN_HLEN;
- }
-
- /* handle L3 protocols */
- if (protocol == htons(ETH_P_IP)) {
- if ((hdr.network - data) > (max_len - sizeof(struct iphdr)))
- return max_len;
-
- /* access ihl as a u8 to avoid unaligned access on ia64 */
- hlen = (hdr.network[0] & 0x0F) << 2;
-
- /* verify hlen meets minimum size requirements */
- if (hlen < sizeof(struct iphdr))
- return hdr.network - data;
-
- /* record next protocol if header is present */
- if (!(hdr.ipv4->frag_off & htons(IP_OFFSET)))
- nexthdr = hdr.ipv4->protocol;
- } else if (protocol == htons(ETH_P_IPV6)) {
- if ((hdr.network - data) > (max_len - sizeof(struct ipv6hdr)))
- return max_len;
-
- /* record next protocol */
- nexthdr = hdr.ipv6->nexthdr;
- hlen = sizeof(struct ipv6hdr);
-#ifdef IXGBE_FCOE
- } else if (protocol == htons(ETH_P_FCOE)) {
- if ((hdr.network - data) > (max_len - FCOE_HEADER_LEN))
- return max_len;
- hlen = FCOE_HEADER_LEN;
-#endif
- } else {
- return hdr.network - data;
- }
-
- /* relocate pointer to start of L4 header */
- hdr.network += hlen;
-
- /* finally sort out TCP/UDP */
- if (nexthdr == IPPROTO_TCP) {
- if ((hdr.network - data) > (max_len - sizeof(struct tcphdr)))
- return max_len;
-
- /* access doff as a u8 to avoid unaligned access on ia64 */
- hlen = (hdr.network[12] & 0xF0) >> 2;
-
- /* verify hlen meets minimum size requirements */
- if (hlen < sizeof(struct tcphdr))
- return hdr.network - data;
-
- hdr.network += hlen;
- } else if (nexthdr == IPPROTO_UDP) {
- if ((hdr.network - data) > (max_len - sizeof(struct udphdr)))
- return max_len;
-
- hdr.network += sizeof(struct udphdr);
- }
-
- /*
- * If everything has gone correctly hdr.network should be the
- * data section of the packet and will be the end of the header.
- * If not then it probably represents the end of the last recognized
- * header.
- */
- if ((hdr.network - data) < max_len)
- return hdr.network - data;
- else
- return max_len;
-}
-
static void ixgbe_set_rsc_gso_size(struct ixgbe_ring *ring,
struct sk_buff *skb)
{
@@ -1793,7 +1679,7 @@ static void ixgbe_pull_tail(struct ixgbe_ring *rx_ring,
* we need the header to contain the greater of either ETH_HLEN or
* 60 bytes if the skb->len is less than 60 for skb_pad.
*/
- pull_len = ixgbe_get_headlen(va, IXGBE_RX_HDR_SIZE);
+ pull_len = eth_get_headlen(va, IXGBE_RX_HDR_SIZE);
/* align pull length to size of long to optimize memcpy performance */
skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
^ permalink raw reply related
* [RFC PATCH 2/3] igb: use new eth_get_headlen interface
From: Alexander Duyck @ 2014-09-04 23:13 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet
In-Reply-To: <20140904230958.12376.2845.stgit@ahduyck-bv4.jf.intel.com>
Update igb to drop the igb_get_headlen function in favor of eth_get_headlen.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 109 -----------------------------
1 file changed, 1 insertion(+), 108 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 89de7fe..4c023f0 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6769,113 +6769,6 @@ static bool igb_is_non_eop(struct igb_ring *rx_ring,
}
/**
- * igb_get_headlen - determine size of header for LRO/GRO
- * @data: pointer to the start of the headers
- * @max_len: total length of section to find headers in
- *
- * This function is meant to determine the length of headers that will
- * be recognized by hardware for LRO, and GRO offloads. The main
- * motivation of doing this is to only perform one pull for IPv4 TCP
- * packets so that we can do basic things like calculating the gso_size
- * based on the average data per packet.
- **/
-static unsigned int igb_get_headlen(unsigned char *data,
- unsigned int max_len)
-{
- union {
- unsigned char *network;
- /* l2 headers */
- struct ethhdr *eth;
- struct vlan_hdr *vlan;
- /* l3 headers */
- struct iphdr *ipv4;
- struct ipv6hdr *ipv6;
- } hdr;
- __be16 protocol;
- u8 nexthdr = 0; /* default to not TCP */
- u8 hlen;
-
- /* this should never happen, but better safe than sorry */
- if (max_len < ETH_HLEN)
- return max_len;
-
- /* initialize network frame pointer */
- hdr.network = data;
-
- /* set first protocol and move network header forward */
- protocol = hdr.eth->h_proto;
- hdr.network += ETH_HLEN;
-
- /* handle any vlan tag if present */
- if (protocol == htons(ETH_P_8021Q)) {
- if ((hdr.network - data) > (max_len - VLAN_HLEN))
- return max_len;
-
- protocol = hdr.vlan->h_vlan_encapsulated_proto;
- hdr.network += VLAN_HLEN;
- }
-
- /* handle L3 protocols */
- if (protocol == htons(ETH_P_IP)) {
- if ((hdr.network - data) > (max_len - sizeof(struct iphdr)))
- return max_len;
-
- /* access ihl as a u8 to avoid unaligned access on ia64 */
- hlen = (hdr.network[0] & 0x0F) << 2;
-
- /* verify hlen meets minimum size requirements */
- if (hlen < sizeof(struct iphdr))
- return hdr.network - data;
-
- /* record next protocol if header is present */
- if (!(hdr.ipv4->frag_off & htons(IP_OFFSET)))
- nexthdr = hdr.ipv4->protocol;
- } else if (protocol == htons(ETH_P_IPV6)) {
- if ((hdr.network - data) > (max_len - sizeof(struct ipv6hdr)))
- return max_len;
-
- /* record next protocol */
- nexthdr = hdr.ipv6->nexthdr;
- hlen = sizeof(struct ipv6hdr);
- } else {
- return hdr.network - data;
- }
-
- /* relocate pointer to start of L4 header */
- hdr.network += hlen;
-
- /* finally sort out TCP */
- if (nexthdr == IPPROTO_TCP) {
- if ((hdr.network - data) > (max_len - sizeof(struct tcphdr)))
- return max_len;
-
- /* access doff as a u8 to avoid unaligned access on ia64 */
- hlen = (hdr.network[12] & 0xF0) >> 2;
-
- /* verify hlen meets minimum size requirements */
- if (hlen < sizeof(struct tcphdr))
- return hdr.network - data;
-
- hdr.network += hlen;
- } else if (nexthdr == IPPROTO_UDP) {
- if ((hdr.network - data) > (max_len - sizeof(struct udphdr)))
- return max_len;
-
- hdr.network += sizeof(struct udphdr);
- }
-
- /* If everything has gone correctly hdr.network should be the
- * data section of the packet and will be the end of the header.
- * If not then it probably represents the end of the last recognized
- * header.
- */
- if ((hdr.network - data) < max_len)
- return hdr.network - data;
- else
- return max_len;
-}
-
-/**
* igb_pull_tail - igb specific version of skb_pull_tail
* @rx_ring: rx descriptor ring packet is being transacted on
* @rx_desc: pointer to the EOP Rx descriptor
@@ -6919,7 +6812,7 @@ static void igb_pull_tail(struct igb_ring *rx_ring,
/* we need the header to contain the greater of either ETH_HLEN or
* 60 bytes if the skb->len is less than 60 for skb_pad.
*/
- pull_len = igb_get_headlen(va, IGB_RX_HDR_LEN);
+ pull_len = eth_get_headlen(va, IGB_RX_HDR_LEN);
/* align pull length to size of long to optimize memcpy performance */
skb_copy_to_linear_data(skb, va, ALIGN(pull_len, sizeof(long)));
^ 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