[PATCH v3] net: hyperv: add support for vlans in netvsc driver

[PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Sriram Krishnan]

Vlan tagged packets are getting dropped when used with DPDK that uses
the AF_PACKET interface on a hyperV guest.

The packet layer uses the tpacket interface to communicate the vlans
information to the upper layers. On Rx path, these drivers can read the
vlan info from the tpacket header but on the Tx path, this information
is still within the packet frame and requires the paravirtual drivers to
push this back into the NDIS header which is then used by the host OS to
form the packet.

This transition from the packet frame to NDIS header is currently missing
hence causing the host OS to drop the all vlan tagged packets sent by
the drivers that use AF_PACKET (ETH_P_ALL) such as DPDK.

Here is an overview of the changes in the vlan header in the packet path:

The RX path (userspace handles everything):
  1. RX VLAN packet is stripped by HOST OS and placed in NDIS header
  2. Guest Kernel RX hv_netvsc packets and moves VLAN info from NDIS
     header into kernel SKB
  3. Kernel shares packets with user space application with PACKET_MMAP.
     The SKB VLAN info is copied to tpacket layer and indication set
     TP_STATUS_VLAN_VALID.
  4. The user space application will re-insert the VLAN info into the frame.

The TX path:
  1. The user space application has the VLAN info in the frame.
  2. Guest kernel gets packets from the application with PACKET_MMAP.
  3. The kernel later sends the frame to the hv_netvsc driver. The only way
     to send VLANs is when the SKB is setup & the VLAN is is stripped from the
     frame.
  4. TX VLAN is re-inserted by HOST OS based on the NDIS header. If it sees
     a VLAN in the frame the packet is dropped.

Cc: xe-linux-external@cisco.com
Cc: Sriram Krishnan <srirakr2@cisco.com>
Signed-off-by: Sriram Krishnan <srirakr2@cisco.com>
---
 drivers/net/hyperv/netvsc_drv.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 6267f706e8ee..2a25b4352369 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -605,6 +605,29 @@ static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
 		*hash_info = hash;
 	}
 
+	/* When using AF_PACKET we need to remove VLAN from frame
+	 * and indicate VLAN information in SKB so HOST OS will
+	 * transmit the VLAN frame
+	 */
+	if (skb->protocol == htons(ETH_P_8021Q)) {
+		u16 vlan_tci = 0;
+		skb_reset_mac_header(skb);
+		if (eth_type_vlan(eth_hdr(skb)->h_proto)) {
+			int pop_err;
+			pop_err = __skb_vlan_pop(skb, &vlan_tci);
+			if (likely(pop_err == 0)) {
+				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
+
+				/* Update the NDIS header pkt lengths */
+				packet->total_data_buflen -= VLAN_HLEN;
+				rndis_msg->msg_len = packet->total_data_buflen;
+				rndis_msg->msg.pkt.data_len = packet->total_data_buflen;
+
+			} else {
+				netdev_err(net,"Pop vlan err %x\n",pop_err);
+			}
+		}
+	}
 	if (skb_vlan_tag_present(skb)) {
 		struct ndis_pkt_8021q_info *vlan;
 
-- 
2.24.0

Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Stephen Hemminger]

On Mon, 20 Jul 2020 22:15:51 +0530
Sriram Krishnan <srirakr2@cisco.com> wrote:

>  
> +	/* When using AF_PACKET we need to remove VLAN from frame
> +	 * and indicate VLAN information in SKB so HOST OS will
> +	 * transmit the VLAN frame
> +	 */
> +	if (skb->protocol == htons(ETH_P_8021Q)) {
> +		u16 vlan_tci = 0;
> +		skb_reset_mac_header(skb);
> +		if (eth_type_vlan(eth_hdr(skb)->h_proto)) {
> +			int pop_err;
> +			pop_err = __skb_vlan_pop(skb, &vlan_tci);
> +			if (likely(pop_err == 0)) {
> +				__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
> +
> +				/* Update the NDIS header pkt lengths */
> +				packet->total_data_buflen -= VLAN_HLEN;
> +				rndis_msg->msg_len = packet->total_data_buflen;
> +				rndis_msg->msg.pkt.data_len = packet->total_data_buflen;
> +
> +			} else {
> +				netdev_err(net,"Pop vlan err %x\n",pop_err);
> +			}
> +		}
> +	}

Minor comments.

1. Blank line between declaration and code.
2. Error handling is different than other parts of this code.
   probably just need a goto drop on error.

It seems like you are putting into message, then driver is putting
it into meta-data in next code block. Maybe it should be combined?

RE: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Haiyang Zhang]

> -----Original Message-----
> From: Sriram Krishnan <srirakr2@cisco.com>
> Sent: Monday, July 20, 2020 12:46 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> Wei Liu <wei.liu@kernel.org>
> Cc: mbumgard@cisco.com; ugm@cisco.com; nimm@cisco.com; xe-linux-
> external@cisco.com; David S. Miller <davem@davemloft.net>; Jakub Kicinski
> <kuba@kernel.org>; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH v3] net: hyperv: add support for vlans in netvsc driver
> 
> Vlan tagged packets are getting dropped when used with DPDK that uses
> the AF_PACKET interface on a hyperV guest.
> 
> The packet layer uses the tpacket interface to communicate the vlans
> information to the upper layers. On Rx path, these drivers can read the
> vlan info from the tpacket header but on the Tx path, this information
> is still within the packet frame and requires the paravirtual drivers to
> push this back into the NDIS header which is then used by the host OS to
> form the packet.
> 
> This transition from the packet frame to NDIS header is currently missing
> hence causing the host OS to drop the all vlan tagged packets sent by
> the drivers that use AF_PACKET (ETH_P_ALL) such as DPDK.
> 
> Here is an overview of the changes in the vlan header in the packet path:
> 
> The RX path (userspace handles everything):
>   1. RX VLAN packet is stripped by HOST OS and placed in NDIS header
>   2. Guest Kernel RX hv_netvsc packets and moves VLAN info from NDIS
>      header into kernel SKB
>   3. Kernel shares packets with user space application with PACKET_MMAP.
>      The SKB VLAN info is copied to tpacket layer and indication set
>      TP_STATUS_VLAN_VALID.
>   4. The user space application will re-insert the VLAN info into the frame.
> 
> The TX path:
>   1. The user space application has the VLAN info in the frame.
>   2. Guest kernel gets packets from the application with PACKET_MMAP.
>   3. The kernel later sends the frame to the hv_netvsc driver. The only way
>      to send VLANs is when the SKB is setup & the VLAN is is stripped from the
>      frame.
>   4. TX VLAN is re-inserted by HOST OS based on the NDIS header. If it sees
>      a VLAN in the frame the packet is dropped.
> 
> Cc: xe-linux-external@cisco.com
> Cc: Sriram Krishnan <srirakr2@cisco.com>
> Signed-off-by: Sriram Krishnan <srirakr2@cisco.com>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 6267f706e8ee..2a25b4352369 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -605,6 +605,29 @@ static int netvsc_xmit(struct sk_buff *skb, struct
> net_device *net, bool xdp_tx)
>  		*hash_info = hash;
>  	}
> 
> +	/* When using AF_PACKET we need to remove VLAN from frame
> +	 * and indicate VLAN information in SKB so HOST OS will
> +	 * transmit the VLAN frame
> +	 */
> +	if (skb->protocol == htons(ETH_P_8021Q)) {
> +		u16 vlan_tci = 0;
> +		skb_reset_mac_header(skb);
> +		if (eth_type_vlan(eth_hdr(skb)->h_proto)) {
> +			int pop_err;
> +			pop_err = __skb_vlan_pop(skb, &vlan_tci);
> +			if (likely(pop_err == 0)) {
> +				__vlan_hwaccel_put_tag(skb,
> htons(ETH_P_8021Q), vlan_tci);
> +
> +				/* Update the NDIS header pkt lengths */
> +				packet->total_data_buflen -= VLAN_HLEN;

packet->total_bytes should be updated too.

Thanks,
- Haiyang

RE: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Haiyang Zhang]

> -----Original Message-----
> From: Sriram Krishnan <srirakr2@cisco.com>
> Sent: Monday, July 20, 2020 12:46 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> Wei Liu <wei.liu@kernel.org>
> Cc: mbumgard@cisco.com; ugm@cisco.com; nimm@cisco.com; xe-linux-
> external@cisco.com; David S. Miller <davem@davemloft.net>; Jakub Kicinski
> <kuba@kernel.org>; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

Also netvsc already supports vlan in "regular" cases. Please be more specific in the subject.
Suggested subject: hv_netvsc: add support for vlans in AF_PACKET mode

Thanks,
- Haiyang

Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[David Miller]

From: Sriram Krishnan <srirakr2@cisco.com>
Date: Mon, 20 Jul 2020 22:15:51 +0530

> +	if (skb->protocol == htons(ETH_P_8021Q)) {
> +		u16 vlan_tci = 0;
> +		skb_reset_mac_header(skb);

Please place an empty line between basic block local variable declarations
and actual code.

> +				netdev_err(net,"Pop vlan err %x\n",pop_err);

A space is necessary before "pop_err".

Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Sriram Krishnan (srirakr2)]

On 21/07/20, 4:57 AM, "David Miller" <davem@davemloft.net> wrote:

    From: Sriram Krishnan <srirakr2@cisco.com>
    Date: Mon, 20 Jul 2020 22:15:51 +0530

    > +	if (skb->protocol == htons(ETH_P_8021Q)) {
    > +		u16 vlan_tci = 0;
    > +		skb_reset_mac_header(skb);

   > Please place an empty line between basic block local variable declarations
   > and actual code.

    > +				netdev_err(net,"Pop vlan err %x\n",pop_err);

    > A space is necessary before "pop_err".

Consolidated list of comments addressed:
> 1. Blank line between declaration and code.
Done

> 2. Error handling is different than other parts of this code.
>   probably just need a goto drop on error.
Done

> It seems like you are putting into message, then driver is putting
> it into meta-data in next code block. Maybe it should be combined?
Not done
This was on purpose. Merging the two code blocks might break existing functionality.
There could be other modes where the packet arrives with 802.1q already in the
Skb and the skb->protocol needn’t be 802.1q.

> packet->total_bytes should be updated too.
Not done.
The total_bytes needs be the total length of packet after the host OS adds the 802.1q header back in
before tx. Updating the total_bytes to -= VLAN_HEADER will lead to packet drop in the Host OS driver.

> Also netvsc already supports vlan in "regular" cases. Please be more specific in the subject.
> Suggested subject: hv_netvsc: add support for vlans in AF_PACKET mode
Done

> A space is necessary before "pop_err".
Done

Uploaded patch v4

RE: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Haiyang Zhang]

> -----Original Message-----
> From: Sriram Krishnan (srirakr2) <srirakr2@cisco.com>
> Sent: Tuesday, July 21, 2020 3:10 AM
> To: David Miller <davem@davemloft.net>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> wei.liu@kernel.org; Malcolm Bumgardner (mbumgard)
> <mbumgard@cisco.com>; Umesha G M (ugm) <ugm@cisco.com>; Niranjan M
> M (nimm) <nimm@cisco.com>; xe-linux-external(mailer list) <xe-linux-
> external@cisco.com>; kuba@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver
> 
> 
> 
> On 21/07/20, 4:57 AM, "David Miller" <davem@davemloft.net> wrote:
> 
>     From: Sriram Krishnan <srirakr2@cisco.com>
>     Date: Mon, 20 Jul 2020 22:15:51 +0530
> 
>     > +	if (skb->protocol == htons(ETH_P_8021Q)) {
>     > +		u16 vlan_tci = 0;
>     > +		skb_reset_mac_header(skb);
> 
>    > Please place an empty line between basic block local variable declarations
>    > and actual code.
> 
>     > +				netdev_err(net,"Pop vlan err %x\n",pop_err);
> 
>     > A space is necessary before "pop_err".
> 
> Consolidated list of comments addressed:
> > 1. Blank line between declaration and code.
> Done
> 
> > 2. Error handling is different than other parts of this code.
> >   probably just need a goto drop on error.
> Done
> 
> > It seems like you are putting into message, then driver is putting it
> > into meta-data in next code block. Maybe it should be combined?
> Not done
> This was on purpose. Merging the two code blocks might break existing
> functionality.
> There could be other modes where the packet arrives with 802.1q already in
> the Skb and the skb->protocol needn’t be 802.1q.
> 
> > packet->total_bytes should be updated too.
> Not done.
> The total_bytes needs be the total length of packet after the host OS adds the
> 802.1q header back in before tx. Updating the total_bytes to -= VLAN_HEADER
> will lead to packet drop in the Host OS driver.

If you make this change, did you see any drop in a live test? The
"packet->total_bytes" in struct hv_netvsc_packet  is for book keeping 
only, which is used for stats info, and not visible by the host at all.

I made this suggestion because the "regular" vlan packet length was 
counted by bytes without the VLAN_HLEN(4) -- the vlan tag is 
in the skb metadata, separately from the ethernet header. I want the 
statistical data for AF_PACKET mode consistent with the regular case.

struct hv_netvsc_packet {
        /* Bookkeeping stuff */
        u8 cp_partial; /* partial copy into send buffer */

        u8 rmsg_size; /* RNDIS header and PPI size */
        u8 rmsg_pgcnt; /* page count of RNDIS header and PPI */
        u8 page_buf_cnt;

        u16 q_idx;
        u16 total_packets;

        u32 total_bytes;
        u32 send_buf_index;
        u32 total_data_buflen;
};

Thanks
- Haiyang

Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

[Sriram Krishnan (srirakr2)]

    On 22/07/20, 3:48 AM, "Haiyang Zhang" <haiyangz@microsoft.com> wrote:
    > If you make this change, did you see any drop in a live test? The
    > "packet->total_bytes" in struct hv_netvsc_packet  is for book keeping
    > only, which is used for stats info, and not visible by the host at all.

    > I made this suggestion because the "regular" vlan packet length was
    > counted by bytes without the VLAN_HLEN(4) -- the vlan tag is
    > in the skb metadata, separately from the ethernet header. I want the
    > statistical data for AF_PACKET mode consistent with the regular case.

    > struct hv_netvsc_packet {
    >         /* Bookkeeping stuff */
    >         u8 cp_partial; /* partial copy into send buffer */

    >         u8 rmsg_size; /* RNDIS header and PPI size */
    >         u8 rmsg_pgcnt; /* page count of RNDIS header and PPI */
    >         u8 page_buf_cnt;

    >         u16 q_idx;
    >         u16 total_packets;

    >         u32 total_bytes;
    >         u32 send_buf_index;
    >         u32 total_data_buflen;
    > };
        
    > Thanks
    > - Haiyang

    Sorry my bad, found out that the drop was a testbed issue and not related to the change. Patch v5 contains the recommendation.

    Thanks,
    Sriram