Message-ID: <CANn89i+y2rO3vvdyg6sjXobewK=GEeo4AFoEy6tnFcaG9y1Q2A@mail.gmail.com>
Date:   Mon, 3 Dec 2018 23:09:43 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [Patch net v3] mlx5: force CHECKSUM_NONE for short ethernet frames

Resent to netdev@ without htm formatting, sorry.

On Mon, Dec 3, 2018 at 11:08 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
>
>
> On Mon, Dec 3, 2018 at 10:48 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>>
>> On Mon, Dec 3, 2018 at 10:34 PM Eric Dumazet <edumazet@...gle.com> wrote:
>> >
>> > On Mon, Dec 3, 2018 at 10:14 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>> > >
>> > > When an ethernet frame is padded to meet the minimum ethernet frame
>> > > size, the padding octets are not covered by the hardware checksum.
>> > > Fortunately the padding octets are ususally zero's, which don't affect
>> > > checksum. However, we have a switch which pads non-zero octets, this
>> > > causes kernel hardware checksum fault repeatedly.
>> > >
>> > > Prior to commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
>> > > skb checksum was forced to be CHECKSUM_NONE when padding is detected.
>> > > After it, we need to keep skb->csum updated, like what we do for RXFCS.
>> > > However, fixing up CHECKSUM_COMPLETE requires to verify and parse IP
>> > > headers, it is not worthy the effort as the packets are so small that
>> > > CHECKSUM_COMPLETE can't save anything.
>> > >
>> > > I tested this patch with RXFCS on and off, it works fine without any
>> > > warning in both cases.
>> > >
>> > > Fixes: 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"),
>> > > Cc: Saeed Mahameed <saeedm@...lanox.com>
>> > > Cc: Eric Dumazet <edumazet@...gle.com>
>> > > Cc: Tariq Toukan <tariqt@...lanox.com>
>> > > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
>> > > ---
>> > >  .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 22 ++++++++++++++++++-
>> > >  1 file changed, 21 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> > > index 624eed345b5d..1c153b8091da 100644
>> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
>> > > @@ -732,6 +732,13 @@ static u8 get_ip_proto(struct sk_buff *skb, int network_depth, __be16 proto)
>> > >                                             ((struct ipv6hdr *)ip_p)->nexthdr;
>> > >  }
>> > >
>> > > +static bool is_short_frame(struct sk_buff *skb, bool has_fcs)
>> > > +{
>> > > +       u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb->len;
>> > > +
>> > > +       return frame_len <= ETH_ZLEN;
>> > > +}
>> > > +
>> > >  static inline void mlx5e_handle_csum(struct net_device *netdev,
>> > >                                      struct mlx5_cqe64 *cqe,
>> > >                                      struct mlx5e_rq *rq,
>> > > @@ -755,9 +762,22 @@ static inline void mlx5e_handle_csum(struct net_device *netdev,
>> > >                 goto csum_unnecessary;
>> > >
>> > >         if (likely(is_last_ethertype_ip(skb, &network_depth, &proto))) {
>> > > +               bool has_fcs = !!(netdev->features & NETIF_F_RXFCS);
>> > > +
>> > >                 if (unlikely(get_ip_proto(skb, network_depth, proto) == IPPROTO_SCTP))
>> > >                         goto csum_unnecessary;
>> > >
>> > > +               /* CQE csum doesn't cover padding octets in short ethernet
>> > > +                * frames. And the pad field is appended prior to calculating
>> > > +                * and appending the FCS field.
>> > > +                *
>> > > +                * Detecting these padded frames requires to verify and parse
>> > > +                * IP headers, so we simply force all those small frames to be
>> > > +                * CHECKSUM_NONE even if they are not padded.
>> > > +                */
>> > > +               if (unlikely(is_short_frame(skb, has_fcs)))
>> > > +                       goto csum_none;
>> >
>> > Should not this go to csum_unnecessary instead ?
>>
>> I don't see why we don't even want to validate the protocol checksum
>> here.
>>
>> Any reason you are suggesting so?
>>
>>
>> >
>> > Probably not a big deal, but small UDP frames might hit this code path,
>> > so ethtool -S would show a lot of csum_none which could confuse mlx5 owners.
>>
>> Why it is confusing? We intentionally bypass hardware checksum
>> and let protocol layer validate it.
>
>
> The hardware has probably validated the L3 & L4 checksum just fine.
>
> Note that if ip_summed is CHECKSUM_UNNECESSARY, the padding bytes (if any)
> have no impact on the csum that has been verified by the NIC.
>
>
>>
>>
>> >
>> > BTW,
>> > It looks like mlx5 prefers delivering skbs with CHECKSUM_COMPLETE instead of
>> > CHECKSUM_UNNECESSARY but at least for ipv6 CHECKSUM_UNNECESSARY
>> > would be slightly faster, by avoiding  various csum_partial() costs
>> > when headers are parsed.
>>
>> Sure, it is certainly faster if you don't want to validate L4 checksum.
>
>
> Sorry I do not get your point.
>
>>
>> The only question is why we don't either validate hardware checksum
>> or L4 checksum?
>>
>
> CHECKSUM_UNNECESSARY means the NIC has fully  validated the checksums,
> including L4 one.
>
> For example, mlx4 drivers first checks if CHECKSUM_UNNECESSARY status can be given,
> and only fallback on CHECKSUM_COMPLETE for the rare cases this CHECKSUM_UNNECESSARY
> status was not given by the NIC.
>
>

Powered by blists - more mailing lists