Re: [PATCH v3] net/packet: support mergeable feature of virtio

["沈安琪(凛玥)" <amy.saq@antgroup.com>]

在 2023/3/10 下午11:01, Willem de Bruijn 写道:
> 沈安琪(凛玥) wrote:
>> 在 2023/3/9 下午11:18, Willem de Bruijn 写道:
>>> 沈安琪(凛玥) wrote:
>>>> 在 2023/3/7 下午11:49, Willem de Bruijn 写道:
>>>>> 沈安琪(凛玥) wrote:
>>>>>> From: Jianfeng Tan <henry.tjf@antgroup.com>
>>>>>>
>>>>>> Packet sockets, like tap, can be used as the backend for kernel vhost.
>>>>>> In packet sockets, virtio net header size is currently hardcoded to be
>>>>>> the size of struct virtio_net_hdr, which is 10 bytes; however, it is not
>>>>>> always the case: some virtio features, such as mrg_rxbuf, need virtio
>>>>>> net header to be 12-byte long.
>>>>>>
>>>>>> Mergeable buffers, as a virtio feature, is worthy of supporting: packets
>>>>>> that are larger than one-mbuf size will be dropped in vhost worker's
>>>>>> handle_rx if mrg_rxbuf feature is not used, but large packets
>>>>>> cannot be avoided and increasing mbuf's size is not economical.
>>>>>>
>>>>>> With this mergeable feature enabled by virtio-user, packet sockets with
>>>>>> hardcoded 10-byte virtio net header will parse mac head incorrectly in
>>>>>> packet_snd by taking the last two bytes of virtio net header as part of
>>>>>> mac header.
>>>>>> This incorrect mac header parsing will cause packet to be dropped due to
>>>>>> invalid ether head checking in later under-layer device packet receiving.
>>>>>>
>>>>>> By adding extra field vnet_hdr_sz with utilizing holes in struct
>>>>>> packet_sock to record currently used virtio net header size and supporting
>>>>>> extra sockopt PACKET_VNET_HDR_SZ to set specified vnet_hdr_sz, packet
>>>>>> sockets can know the exact length of virtio net header that virtio user
>>>>>> gives.
>>>>>> In packet_snd, tpacket_snd and packet_recvmsg, instead of using
>>>>>> hardcoded virtio net header size, it can get the exact vnet_hdr_sz from
>>>>>> corresponding packet_sock, and parse mac header correctly based on this
>>>>>> information to avoid the packets being mistakenly dropped.
>>>>>>
>>>>>> Besides, has_vnet_hdr field in struct packet_sock is removed since all
>>>>>> the information it provides is covered by vnet_hdr_sz field: a packet
>>>>>> socket has a vnet header if and only if its vnet_hdr_sz is not zero.
>>>>>>
>>>>>> Signed-off-by: Jianfeng Tan <henry.tjf@antgroup.com>
>>>>>> Co-developed-by: Anqi Shen <amy.saq@antgroup.com>
>>>>>> Signed-off-by: Anqi Shen <amy.saq@antgroup.com>
>>>>>> ---
>>>>>> diff --git a/net/packet/internal.h b/net/packet/internal.h
>>>>>> index 48af35b..9b52d93 100644
>>>>>> --- a/net/packet/internal.h
>>>>>> +++ b/net/packet/internal.h
>>>>>> @@ -119,9 +119,9 @@ struct packet_sock {
>>>>>>     	unsigned int		running;	/* bind_lock must be held */
>>>>>>     	unsigned int		auxdata:1,	/* writer must hold sock lock */
>>>>>>     				origdev:1,
>>>>>> -				has_vnet_hdr:1,
>>>>>>     				tp_loss:1,
>>>>>> -				tp_tx_has_off:1;
>>>>>> +				tp_tx_has_off:1,
>>>>>> +				vnet_hdr_sz:8;
>>>>> just a separate u8 variable , rather than 8 bits in a u32.
>>>>>
>>>>>>     	int			pressure;
>>>>>>     	int			ifindex;	/* bound device		*/
>>>> We plan to add
>>>>
>>>> +	   u8	vnet_hdr_sz:8;
>>>>
>>>> here.
>>>> Is this a proper place to add this field to make sure the cacheline will not be broken?
>>> When in doubt, use pahole (`pahole -C packet_sock net/packet/af_packet.o`).
>>>
>>> There currently is a 27-bit hole before pressure. That would be a good spot.
>>
>> Thanks for the advice! We will try the tool.
>>
>> Besides, we wonder whether it will be better to use unsigned char or u8
>> here to be more consistent with other fields.
> u8 is good


Got it. Thanks. We will address the comments and send out v4 soon.