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

[Haiyang Zhang <haiyangz@microsoft.com>]

> -----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