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

[Bin Meng]

On Fri, Mar 12, 2021 at 2:50 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2021/3/12 2:28 下午, Bin Meng wrote:
> > On Fri, Mar 12, 2021 at 2:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 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.
> >>
> > Literally I don't see Peter requested option (1) to be done in net
> > core or net devs.
> >
> > If doing it in netdevs, the following codes need to be duplicated in
> > both SLiRP and TAP codes.
> >
> > if (sender->info->type == NET_CLIENT_DRIVER_USER ||
> >      sender->info->type == NET_CLIENT_DRIVER_TAP) {
> >      do the short frames padding;
> > }
>
>
> So my understanding is that it's better to be done at netdev where we
> know whether it's a ethernet dev, core should be protocol independent.

OK, will change to pad short frames in SLiRP and TAP codes in the next version.

Regards,
Bin