Jason Wang
2022-Apr-20 02:47 UTC
[PATCH net 1/2] net/af_packet: adjust network header position for VLAN tagged packets
On Wed, Apr 20, 2022 at 9:00 AM Hangbin Liu <liuhangbin at gmail.com> wrote:> > On Tue, Apr 19, 2022 at 10:26:09AM -0400, Michael S. Tsirkin wrote: > > > > There are also some duplicated codes in these *_snd functions. > > > > I think we can move them out to one single function. > > > > > > Please don't refactor this code. It will complicate future backports > > > of stable fixes. > > > > Hmm I don't know offhand which duplication this refers to specifically > > so maybe it's not worth addressing specifically but generally not > > cleaning up code just because of backports seems wrong ... > > Yes, packet_snd() and tpacket_snd() share same addr/msg checking logic that > I think we can clean up. > > > > > > stretching the definition of the flags to include VLAN is acceptable > > > > > (unlike outright tunnels), but even then I would suggest for net-next. > > > > > > > > As I asked, I'm not familiar with virtio code. Do you think if I should > > > > add a new VIRTIO_NET_HDR_GSO_VLAN flag? It's only a L2 flag without any L3 > > > > info. If I add something like VIRTIO_NET_HDR_GSO_VLAN_TCPV4/TCPV6/UDP. That > > > > would add more combinations. Which doesn't like a good idea. > > > > > > I would prefer a new flag to denote this type, so that we can be > > > strict and only change the datapath for packets that have this flag > > > set (and thus express the intent). > > > > > > But the VIRTIO_NET_HDR types are defined in the virtio spec. The > > > maintainers should probably chime in. > > > > Yes, it's a UAPI extension, not to be done lightly. In this case IIUC > > gso_type in the header is only u8 - 8 bits and 5 of these are already > > used. So I don't think the virtio TC will be all that happy to burn up > > a bit unless a clear benefit can be demonstrated. > > > > I agree with the net-next proposal, I think it's more a feature than a > > bugfix. In particular I think a Fixes tag can also be dropped in that > > IIUC GSO for vlan packets didn't work even before that commit - right?It should work, we initialize vlan_features since ("4fda830263c5 virtio-net: initialize vlan_features"). What we don't support is vlan offload.> > Right. virtio_net_hdr GSO with vlan doesn't work before.It doesn't work since we don't support that feature.> I will post this to net-next.If you want to do that, you need a spec patch as well. Thanks> > Thanks > Hangbin >