Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP

From: Jason Wang <jasowang@redhat.com>
To: Bin Meng <bmeng.cn@gmail.com>, Peter Maydell <peter.maydell@linaro.org>
Cc: "Dmitry Fleytman" <dmitry.fleytman@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Bin Meng" <bin.meng@windriver.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Stefan Hajnoczi" <stefanha@redhat.com>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [RFC PATCH v3 02/10] net: Pad short frames to minimum size before send from SLiRP/TAP
Date: Fri, 12 Mar 2021 14:22:54 +0800	[thread overview]
Message-ID: <4cc1bcb9-2288-ac03-4a5f-a1ebad4d34ef@redhat.com> (raw)
In-Reply-To: <CAEUhbmWPWA_0wWBzacu1K2Kx+A2dgmFs89TQrbcG52mX=P5o7Q@mail.gmail.com>

On 2021/3/11 6:27 下午, Bin Meng wrote:
> On Thu, Mar 11, 2021 at 6:22 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Thu, 11 Mar 2021 at 09:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> On Thu, Mar 11, 2021 at 5:43 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> On Thu, 11 Mar 2021 at 03:01, Jason Wang <jasowang@redhat.com> wrote:
>>>>> And after a discussion 10 years ago [1]. Michael (cced) seems to want to
>>>>> keep the padding logic in the NIC itself (probably with a generic helper
>>>>> in the core). Since 1) the padding is only required for ethernet 2)
>>>>> virito-net doesn't need that (it can pass incomplete packet by design).
>>>> Like I said, we need to decide; either:
>>>>
>>>>   (1) we do want to support short packets in the net core:
>>>> every sender needs to either pad, or to have some flag to say
>>>> "my implementation can't pad, please can the net core do it for me",
>>>> unless they are deliberately sending a short packet. Every
>>>> receiver needs to be able to cope with short packets, at least
>>>> in the sense of not crashing (they should report them as a rx
>>>> error if they have that kind of error reporting status register).
>>>> I think we have mostly implemented our NIC models this way.
>>>>
>>>>   (2) we simply don't support short packets in the net core:
>>>> nobody (not NICs, not network backends) needs to pad, because
>>>> they can rely on the core to do it. Some existing senders and
>>>> receivers may have now-dead code to do their own padding which
>>>> could be removed.
>>>>
>>>> MST is advocating for (1) in that old thread. That's a coherent
>>>> position.
>>> But it's a wrong approach. As Edgar and Stefan also said in the old
>>> discussion thread, padding in the RX is wrong as real world NICs don't
>>> do this.
>> Neither option (1) nor option (2) involve padding in RX.
> Correct. What I referred to is the current approach used in many NIC
> modes, which is wrong, and we have to correct this.
>
>> Option (1) is:
>>   * no NIC implementation pads on TX, except as defined
>>     by whatever NIC-specific config registers or h/w behaviour
>>     might require (ie if the guest wants to send a short packet
>>     it can do that)
>>   * non-NIC sources like slirp need to pad on TX unless they're
>>     deliberately trying to send a short packet
>>   * all receivers of packets need to cope with being given a
>>     short packet; this is usually going to mean "flag it to the
>>     guest as an RX error", but exact behaviour is NIC-dependent
>>
> My patch series in RFC v2/v3 does almost exactly this option (1),
> except "flag it to the guest as an RX error".

Is it? You did it at net core instead of netdevs if I read the code 
correctly.

>
>> Option (2) is:
>>   * the net core code pads any packet that goes through it
>>   * no NIC implementation needs to pad on TX (it is harmless if they do)
>>   * non-NIC sources don't need to pad on TX
>>   * no receivers of packets need to cope with being given short packets
>>
>> Option 1 is what the real world does. Option 2 is a simplification
>> which throws away the ability to emulate handling of short packets,
>> in exchange for not having to sort out senders like slirp and not
>> having to be so careful about short-packet handling in NIC models.
>>
>> If MST is correct that some use cases require short-packet support,
>> then we need to go for option 1, I think.

For NIC RX, I wonder whether we can introduce a boolean to whether it 
requries padding. And then the netdevs can only do the padding when it's 
required. E.g virito-net doesn't need padding.

Thanks

> Regards,
> Bin
>