On Tue, 2018-11-27 at 16:07 -0800, Cong Wang wrote:
> On Tue, Nov 27, 2018 at 3:48 PM Eric Dumazet <eric.duma...@gmail.com>
> wrote:
> >
> >
> > On 11/27/2018 03:21 PM, Cong Wang 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 is forced to be CHECKSUM_NONE when padding is
> > > detected.
> > > After it, we have to keep skb->csum updated, like what we do for
> > > FCS.
> > >
> > > The logic is a bit complicated when dealing with both FCS and
> > > padding,
> > > so I wrap it up in a helper function mlx5e_csum_padding().
> > >
> > > 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 <sae...@mellanox.com>
> > > Cc: Eric Dumazet <eduma...@google.com>
> > > Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com>
> > > ---
> > > .../net/ethernet/mellanox/mlx5/core/en_rx.c | 36
> > > +++++++++++++++++++
> > > 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > index 16985ca3248d..93c18647ca74 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> > > @@ -732,6 +732,35 @@ static u8 get_ip_proto(struct sk_buff *skb,
> > > __be16 proto)
> > > ((struct ipv6hdr
> > > *)ip_p)->nexthdr;
> > > }
> > >
> > > +static void mlx5e_csum_padding(struct sk_buff *skb, int
> > > network_depth,
> > > + __be16 proto, bool has_fcs)
> > > +{
> > > + u32 frame_len = has_fcs ? skb->len - ETH_FCS_LEN : skb-
> > > >len;
> > > + void *ip_p = skb->data + network_depth;
> > > + u32 pad_offset, pad_len;
> > > + void *pad;
> > > +
> > > + if (likely(frame_len > ETH_ZLEN))
> > > + return;
> > > +
> >
> > But the padding might be added on normal packets (say 1000 bytes +
> > 3 bytes of padding) ?
>
> I never see other padding cases than ETH_ZLEN. Does ethernet standard
> require padding for other cases? I only read the section "3.2.8 Pad
> field" in
> the standard.
>
hmm, so we are going to parse each and every packets ? to find if there
is an end padding ? that is costly (packet rate wise)!
if the ETH_ZLEN check is good enough for now, let's stick with that.
We are currently exploring our options if we can W/A this in HW/FW and
avoid the extra driver overhead.
> > The bug here is that mlx5 csum only includes the data in IP frame.
> >
> > I would simply force skb->ip_summed to CHECKSUM_NONE if any padding
> > is detected.
> >
Totally agree, let's just skip csum completes for very small packets
( < ETH_ZLEN ).
> > Otherwise, your patch needs more work when multiple frags are used
> > (ie num_frags > 1 )
>
> You mean using skb_header_pointer()? Yeah if we need to handle cases
> larger than ETH_ZLEN.
>
> Thanks.
