Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: restore vlan header on user read). This patch adds hardware vlan tx support for tuntap. This is done by copying vlan header directly into userspace in tun_put_user() instead of doing it through __vlan_put_tag() in dev_hard_start_xmit(). This eliminates one unnecessary memove in vlan_insert_tag() for 802.1ad and 802.1q traffic. pktgen test shows about 20% improvement for 802.1q traffic: Before: 662149pps 317Mb/sec (317831520bps) errors: 0 After: 801033pps 384Mb/sec (384495840bps) errors: 0 Cc: Basil Gor <basil.gor@gmail.com> Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- drivers/net/tun.c | 39 +++++++++++++++++++++++++++++++++++---- 1 files changed, 35 insertions(+), 4 deletions(-) diff --git a/drivers/net/tun.c b/drivers/net/tun.c index a72d141..66e265d 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -60,6 +60,7 @@ #include <linux/if_arp.h> #include <linux/if_ether.h> #include <linux/if_tun.h> +#include <linux/if_vlan.h> #include <linux/crc32.h> #include <linux/nsproxy.h> #include <linux/virtio_net.h> @@ -1265,6 +1266,7 @@ static ssize_t tun_put_user(struct tun_struct *tun, { struct tun_pi pi = { 0, skb->protocol }; ssize_t total = 0; + int vlan_offset = 0; if (!(tun->flags & TUN_NO_PI)) { if ((len -= sizeof(pi)) < 0) @@ -1328,11 +1330,39 @@ static ssize_t tun_put_user(struct tun_struct *tun, total += tun->vnet_hdr_sz; } - len = min_t(int, skb->len, len); + if (!vlan_tx_tag_present(skb)) + len = min_t(int, skb->len, len); + else { + int copy, ret; + struct { + __be16 h_vlan_proto; + __be16 h_vlan_TCI; + } veth; + veth.h_vlan_proto = skb->vlan_proto; + veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb)); + + vlan_offset = offsetof(struct vlan_ethhdr, h_vlan_proto); + len = min_t(int, skb->len + VLAN_HLEN, len); + + copy = min_t(int, vlan_offset, len); + ret = skb_copy_datagram_const_iovec(skb, 0, iv, total, copy); + len -= copy; + total += copy; + if (ret || !len) + goto done; + + copy = min_t(int, sizeof(veth), len); + ret = memcpy_toiovecend(iv, (void *)&veth, total, copy); + len -= copy; + total += copy; + if (ret || !len) + goto done; + } - skb_copy_datagram_const_iovec(skb, 0, iv, total, len); - total += skb->len; + skb_copy_datagram_const_iovec(skb, vlan_offset, iv, total, len); + total += len; +done: tun->dev->stats.tx_packets++; tun->dev->stats.tx_bytes += len; @@ -1691,7 +1721,8 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr) tun_flow_init(tun); dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | - TUN_USER_FEATURES; + TUN_USER_FEATURES | NETIF_F_HW_VLAN_CTAG_TX | + NETIF_F_HW_VLAN_STAG_TX; dev->features = dev->hw_features; dev->vlan_features = dev->features; -- 1.7.1
Hello. On 23-07-2013 11:15, Jason Wang wrote: > Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: restore > vlan header on user read). This patch adds hardware vlan tx support for > tuntap. This is done by copying vlan header directly into userspace in > tun_put_user() instead of doing it through __vlan_put_tag() in > dev_hard_start_xmit(). This eliminates one unnecessary memove in s/memove/memmove/? > vlan_insert_tag() for 802.1ad and 802.1q traffic. > pktgen test shows about 20% improvement for 802.1q traffic: > Before: > 662149pps 317Mb/sec (317831520bps) errors: 0 > After: > 801033pps 384Mb/sec (384495840bps) errors: 0 > Cc: Basil Gor <basil.gor@gmail.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > drivers/net/tun.c | 39 +++++++++++++++++++++++++++++++++++---- > 1 files changed, 35 insertions(+), 4 deletions(-) > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index a72d141..66e265d 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c [...] > @@ -1328,11 +1330,39 @@ static ssize_t tun_put_user(struct tun_struct *tun, > total += tun->vnet_hdr_sz; > } > > - len = min_t(int, skb->len, len); > + if (!vlan_tx_tag_present(skb)) > + len = min_t(int, skb->len, len); > + else { According to Documentation/CodingStyle chapter 3, both arms of an *if* statement should have {} if one arm has it. > + int copy, ret; > + struct { > + __be16 h_vlan_proto; > + __be16 h_vlan_TCI; > + } veth; Empty line wouldn't hurt here, after declarations... > + veth.h_vlan_proto = skb->vlan_proto; > + veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb)); [...] WBR, Sergei
On Tue, 23 Jul 2013 15:15:48 +0800
Jason Wang <jasowang@redhat.com> wrote:
> Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: restore
> vlan header on user read). This patch adds hardware vlan tx support for
> tuntap. This is done by copying vlan header directly into userspace in
> tun_put_user() instead of doing it through __vlan_put_tag() in
> dev_hard_start_xmit(). This eliminates one unnecessary memove in
> vlan_insert_tag() for 802.1ad and 802.1q traffic.
>
> pktgen test shows about 20% improvement for 802.1q traffic:
>
> Before:
> 662149pps 317Mb/sec (317831520bps) errors: 0
> After:
> 801033pps 384Mb/sec (384495840bps) errors: 0
>
> Cc: Basil Gor <basil.gor@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
You need to make this configurable by some mechanism, since otherwise
it will break applications that expect current VLAN behavior
On Tue, Jul 23, 2013 at 08:17:27AM -0700, Stephen Hemminger wrote:
> On Tue, 23 Jul 2013 15:15:48 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
> > Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: restore
> > vlan header on user read). This patch adds hardware vlan tx support for
> > tuntap. This is done by copying vlan header directly into userspace in
> > tun_put_user() instead of doing it through __vlan_put_tag() in
> > dev_hard_start_xmit(). This eliminates one unnecessary memove in
> > vlan_insert_tag() for 802.1ad and 802.1q traffic.
> >
> > pktgen test shows about 20% improvement for 802.1q traffic:
> >
> > Before:
> > 662149pps 317Mb/sec (317831520bps) errors: 0
> > After:
> > 801033pps 384Mb/sec (384495840bps) errors: 0
> >
> > Cc: Basil Gor <basil.gor@gmail.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
>
> You need to make this configurable by some mechanism, since otherwise
> it will break applications that expect current VLAN behavior
I don't think there's a behavior change: vlan is still linearized
into the buffer. It's just done more efficiently.
On Tue, 23 Jul 2013 15:15:48 +0800
Jason Wang <jasowang@redhat.com> wrote:
> + struct {
> + __be16 h_vlan_proto;
> + __be16 h_vlan_TCI;
> + } veth;
Don't you want to use struct vlan_hdr here?
Your definition puts the two fields out of order?
On 07/23/2013 08:12 PM, Sergei Shtylyov wrote: > Hello. > > On 23-07-2013 11:15, Jason Wang wrote: > >> Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: >> restore >> vlan header on user read). This patch adds hardware vlan tx support for >> tuntap. This is done by copying vlan header directly into userspace in >> tun_put_user() instead of doing it through __vlan_put_tag() in >> dev_hard_start_xmit(). This eliminates one unnecessary memove in > > s/memove/memmove/? > Yes. >> vlan_insert_tag() for 802.1ad and 802.1q traffic. > >> pktgen test shows about 20% improvement for 802.1q traffic: > >> Before: >> 662149pps 317Mb/sec (317831520bps) errors: 0 >> After: >> 801033pps 384Mb/sec (384495840bps) errors: 0 > >> Cc: Basil Gor <basil.gor@gmail.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> drivers/net/tun.c | 39 +++++++++++++++++++++++++++++++++++---- >> 1 files changed, 35 insertions(+), 4 deletions(-) > >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index a72d141..66e265d 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c > [...] >> @@ -1328,11 +1330,39 @@ static ssize_t tun_put_user(struct tun_struct >> *tun, >> total += tun->vnet_hdr_sz; >> } >> >> - len = min_t(int, skb->len, len); >> + if (!vlan_tx_tag_present(skb)) >> + len = min_t(int, skb->len, len); >> + else { > > According to Documentation/CodingStyle chapter 3, both arms of an > *if* statement should have {} if one arm has it. > Right. >> + int copy, ret; >> + struct { >> + __be16 h_vlan_proto; >> + __be16 h_vlan_TCI; >> + } veth; > > Empty line wouldn't hurt here, after declarations... > Ok >> + veth.h_vlan_proto = skb->vlan_proto; >> + veth.h_vlan_TCI = htons(vlan_tx_tag_get(skb)); > [...] > > WBR, Sergei > Thanks, will correct them. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/23/2013 11:17 PM, Stephen Hemminger wrote:
> On Tue, 23 Jul 2013 15:15:48 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> Inspired by commit f09e2249c4f5c7c13261ec73f5a7807076af0c8e (macvtap: restore
>> vlan header on user read). This patch adds hardware vlan tx support for
>> tuntap. This is done by copying vlan header directly into userspace in
>> tun_put_user() instead of doing it through __vlan_put_tag() in
>> dev_hard_start_xmit(). This eliminates one unnecessary memove in
>> vlan_insert_tag() for 802.1ad and 802.1q traffic.
>>
>> pktgen test shows about 20% improvement for 802.1q traffic:
>>
>> Before:
>> 662149pps 317Mb/sec (317831520bps) errors: 0
>> After:
>> 801033pps 384Mb/sec (384495840bps) errors: 0
>>
>> Cc: Basil Gor <basil.gor@gmail.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> You need to make this configurable by some mechanism, since otherwise
> it will break applications that expect current VLAN behavior
> --
>
Didn't see any breakage. Vlan id will always exist in the userspace buffer.
On 07/23/2013 11:53 PM, Stephen Hemminger wrote: > On Tue, 23 Jul 2013 15:15:48 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> + struct { >> + __be16 h_vlan_proto; >> + __be16 h_vlan_TCI; >> + } veth; > Don't you want to use struct vlan_hdr here? There's no need to care encapsulated proto here. In fact, we just emulate the hardware inserting of 802.1Q header. So only skb->vlan_tci and skb->vlan_proto needs to be cared. > Your definition puts the two fields out of order? It's order is same as struct vlan_ethhdr. Did you see any issue?
On Wed, 24 Jul 2013 13:55:14 +0800
Jason Wang <jasowang@redhat.com> wrote:
> On 07/23/2013 11:53 PM, Stephen Hemminger wrote:
> > On Tue, 23 Jul 2013 15:15:48 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> >> + struct {
> >> + __be16 h_vlan_proto;
> >> + __be16 h_vlan_TCI;
> >> + } veth;
> > Don't you want to use struct vlan_hdr here?
>
> There's no need to care encapsulated proto here. In fact, we just
> emulate the hardware inserting of 802.1Q header. So only skb->vlan_tci
> and skb->vlan_proto needs to be cared.
> > Your definition puts the two fields out of order?
>
> It's order is same as struct vlan_ethhdr. Did you see any issue?
I was looking at if_vlan.h struct vlan_hdr. The veth structure
is a slice between ether and vlan header.
On 07/24/2013 11:11 PM, Stephen Hemminger wrote:
> On Wed, 24 Jul 2013 13:55:14 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 07/23/2013 11:53 PM, Stephen Hemminger wrote:
>>> On Tue, 23 Jul 2013 15:15:48 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>> + struct {
>>>> + __be16 h_vlan_proto;
>>>> + __be16 h_vlan_TCI;
>>>> + } veth;
>>> Don't you want to use struct vlan_hdr here?
>> There's no need to care encapsulated proto here. In fact, we just
>> emulate the hardware inserting of 802.1Q header. So only skb->vlan_tci
>> and skb->vlan_proto needs to be cared.
>>> Your definition puts the two fields out of order?
>> It's order is same as struct vlan_ethhdr. Did you see any issue?
> I was looking at if_vlan.h struct vlan_hdr. The veth structure
> is a slice between ether and vlan header.
Right, so no functional issue.