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: [email protected]
Cc: Sriram Krishnan <[email protected]>
Signed-off-by: Sriram Krishnan <[email protected]>
---
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
On Mon, 20 Jul 2020 22:15:51 +0530
Sriram Krishnan <[email protected]> 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?
> -----Original Message-----
> From: Sriram Krishnan <[email protected]>
> Sent: Monday, July 20, 2020 12:46 PM
> To: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Stephen Hemminger <[email protected]>;
> Wei Liu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; xe-linux-
> [email protected]; David S. Miller <[email protected]>; Jakub Kicinski
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> 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: [email protected]
> Cc: Sriram Krishnan <[email protected]>
> Signed-off-by: Sriram Krishnan <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Sriram Krishnan <[email protected]>
> Sent: Monday, July 20, 2020 12:46 PM
> To: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Stephen Hemminger <[email protected]>;
> Wei Liu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected]; xe-linux-
> [email protected]; David S. Miller <[email protected]>; Jakub Kicinski
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> 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
From: Sriram Krishnan <[email protected]>
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".
On 21/07/20, 4:57 AM, "David Miller" <[email protected]> wrote:
From: Sriram Krishnan <[email protected]>
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
> -----Original Message-----
> From: Sriram Krishnan (srirakr2) <[email protected]>
> Sent: Tuesday, July 21, 2020 3:10 AM
> To: David Miller <[email protected]>
> Cc: KY Srinivasan <[email protected]>; Haiyang Zhang
> <[email protected]>; Stephen Hemminger <[email protected]>;
> [email protected]; Malcolm Bumgardner (mbumgard)
> <[email protected]>; Umesha G M (ugm) <[email protected]>; Niranjan M
> M (nimm) <[email protected]>; xe-linux-external(mailer list) <xe-linux-
> [email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver
>
>
>
> On 21/07/20, 4:57 AM, "David Miller" <[email protected]> wrote:
>
> From: Sriram Krishnan <[email protected]>
> 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
On 22/07/20, 3:48 AM, "Haiyang Zhang" <[email protected]> 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