From: David Miller [mailto:[email protected]]
Sent: 10 listopada 2016 18:01
To: Rafal Ozieblo
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH net-next v5]] cadence: Add LSO support.
From: Rafal Ozieblo <[email protected]>
Date: Wed, 9 Nov 2016 13:41:02 +0000
> First, please remove the spurious closing bracket in your Subject line in future submittions.
>
>> + if (is_udp) /* is_udp is only set when (is_lso) is checked */
>> + /* zero UDP checksum, not calculated by h/w for UFO */
>> + udp_hdr(skb)->check = 0;
>
> This is really not ok.
>
> If UFO is in use it should not silently disable UDP checksums.
>
> If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
According Cadence Gigabit Ethernet MAC documentation:
"Hardware will not calculate the UDP checksum or modify the UDP checksum field. Therefore software
must set a value of zero in the checksum field in the UDP header (in the first payload buffer)
to indicate to the receiver that the UDP datagram does not include a checksum."
It is hardware requirement.
On Mon, 2016-11-14 at 09:32 +0000, Rafal Ozieblo wrote:
> From: David Miller [mailto:[email protected]]
> Sent: 10 listopada 2016 18:01
> To: Rafal Ozieblo
> Cc: [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH net-next v5]] cadence: Add LSO support.
>
> From: Rafal Ozieblo <[email protected]>
> Date: Wed, 9 Nov 2016 13:41:02 +0000
>
> > First, please remove the spurious closing bracket in your Subject line in future submittions.
> >
> >> + if (is_udp) /* is_udp is only set when (is_lso) is checked */
> >> + /* zero UDP checksum, not calculated by h/w for UFO */
> >> + udp_hdr(skb)->check = 0;
> >
> > This is really not ok.
> >
> > If UFO is in use it should not silently disable UDP checksums.
> >
> > If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
>
> According Cadence Gigabit Ethernet MAC documentation:
>
> "Hardware will not calculate the UDP checksum or modify the UDP checksum field. Therefore software
> must set a value of zero in the checksum field in the UDP header (in the first payload buffer)
> to indicate to the receiver that the UDP datagram does not include a checksum."
>
> It is hardware requirement.
Then do not claim NETIF_F_UFO suport in the driver, if hardware is
absolutely not capable of handling this.
Linux stack will provide proper udp checksum.
Almost no driver sets NETIF_F_UFO.
From: Rafal Ozieblo <[email protected]>
Date: Mon, 14 Nov 2016 09:32:34 +0000
>> If UFO is in use it should not silently disable UDP checksums.
>>
>> If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
>
> According Cadence Gigabit Ethernet MAC documentation:
>
> "Hardware will not calculate the UDP checksum or modify the UDP checksum field. Therefore software
> must set a value of zero in the checksum field in the UDP header (in the first payload buffer)
> to indicate to the receiver that the UDP datagram does not include a checksum."
>
> It is hardware requirement.
I do not doubt that it is a hardware restriction.
But I am saying that you cannot enable this feature under Linux if
this is how it operates on your hardware.
> > > If UFO is in use it should not silently disable UDP checksums.
> > >
> > > If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
> >
> > According Cadence Gigabit Ethernet MAC documentation:
> >
> > "Hardware will not calculate the UDP checksum or modify the UDP
> > checksum field. Therefore software must set a value of zero in the
> > checksum field in the UDP header (in the first payload buffer) to indicate to the receiver that the UDP datagram does not include a checksum."
> >
> > It is hardware requirement.
>
> I do not doubt that it is a hardware restriction.
>
> But I am saying that you cannot enable this feature under Linux if this is how it operates on your hardware.
Would it be good to enable UFO conditionally with some internal define? Ex.:
+#ifdef MACB_ENABLE_UFO
+#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
+#else
+#define MACB_NETIF_LSO (NETIF_F_TSO)
+#endif
I could add precise comment here that ufo is possible only without checksum.
Or maybe I could enable it from module_params or device-tree (like: drivers/net/ethernet/neterion/s2io.c).
On Tue, 2016-11-15 at 07:07 +0000, Rafal Ozieblo wrote:
> > > > If UFO is in use it should not silently disable UDP checksums.
> > > >
> > > > If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
> > >
> > > According Cadence Gigabit Ethernet MAC documentation:
> > >
> > > "Hardware will not calculate the UDP checksum or modify the UDP
> > > checksum field. Therefore software must set a value of zero in the
> > > checksum field in the UDP header (in the first payload buffer) to indicate to the receiver that the UDP datagram does not include a checksum."
> > >
> > > It is hardware requirement.
> >
> > I do not doubt that it is a hardware restriction.
> >
> > But I am saying that you cannot enable this feature under Linux if this is how it operates on your hardware.
>
> Would it be good to enable UFO conditionally with some internal define? Ex.:
>
> +#ifdef MACB_ENABLE_UFO
> +#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
> +#else
> +#define MACB_NETIF_LSO (NETIF_F_TSO)
> +#endif
>
> I could add precise comment here that ufo is possible only without checksum.
>
> Or maybe I could enable it from module_params or device-tree (like: drivers/net/ethernet/neterion/s2io.c).
No you can not do that.
1) That would violate UDP specs.
2) Module params are no longer accepted.
3) Comments in a driver source code would only help the driver
maintainer, not users to make their mind.
Only way would be to propagate the intent of the sender.
Only the sender application can decide to generate UDP checksums or not.
Your driver ndo_features_check() could then force software segmentation
fallback if the user did not asked to disable UDP checksums, and packet
is UFO.
(look for UDP_NO_CHECK6_TX, and SO_NO_CHECK )
Problem is complex, because the skb has no marker, only the socket has.
And socket state could change between packets, and packets can stay in
an intermediate qdisc before hitting device driver. So looking at
skb->sk from your ndo_features_check() would be racy.
What use case would you have precisely ?
> From: Eric Dumazet [mailto:[email protected]]
> Sent: 15 listopada 2016 14:12
> To: Rafal Ozieblo
> Cc: David Miller; [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH net-next v5] cadence: Add LSO support.
>
> On Tue, 2016-11-15 at 07:07 +0000, Rafal Ozieblo wrote:
> > > > > If UFO is in use it should not silently disable UDP checksums.
> > > > >
> > > > > If you cannot support UFO with proper checksumming, then you cannot enable support for that feature.
> > > >
> > > > According Cadence Gigabit Ethernet MAC documentation:
> > > >
> > > > "Hardware will not calculate the UDP checksum or modify the UDP
> > > > checksum field. Therefore software must set a value of zero in the
> > > > checksum field in the UDP header (in the first payload buffer) to indicate to the receiver that the UDP datagram does not include a checksum."
> > > >
> > > > It is hardware requirement.
> > >
> > > I do not doubt that it is a hardware restriction.
> > >
> > > But I am saying that you cannot enable this feature under Linux if this is how it operates on your hardware.
> >
> > Would it be good to enable UFO conditionally with some internal define? Ex.:
> >
> > +#ifdef MACB_ENABLE_UFO
> > +#define MACB_NETIF_LSO (NETIF_F_TSO | NETIF_F_UFO)
> > +#else
> > +#define MACB_NETIF_LSO (NETIF_F_TSO)
> > +#endif
> >
> > I could add precise comment here that ufo is possible only without checksum.
> >
> > Or maybe I could enable it from module_params or device-tree (like: drivers/net/ethernet/neterion/s2io.c).
>
> No you can not do that.
>
> 1) That would violate UDP specs.
> 2) Module params are no longer accepted.
> 3) Comments in a driver source code would only help the driver maintainer, not users to make their mind.
>
> Only way would be to propagate the intent of the sender.
>
> Only the sender application can decide to generate UDP checksums or not.
>
> Your driver ndo_features_check() could then force software segmentation fallback if the user did not asked to disable UDP checksums, and packet is UFO.
>
> (look for UDP_NO_CHECK6_TX, and SO_NO_CHECK )
>
> Problem is complex, because the skb has no marker, only the socket has.
>
> And socket state could change between packets, and packets can stay in an intermediate qdisc before hitting device driver. So looking at
> skb->sk from your ndo_features_check() would be racy.
>
> What use case would you have precisely ?
I have talked with hardware team who designed and created gem IP. The conclusion is that there is no need to zeroed checksum for UFO.
I have tested UFO without zeroing UDP checksum on cadence gem. It works fine.
I'll deeply investigate and test UFO again. I'll send version 6 of LSO PATCH either with UFO without zeroing or without UFO at all.
From: Rafal Ozieblo <[email protected]>
Date: Tue, 15 Nov 2016 07:07:14 +0000
> Would it be good to enable UFO conditionally with some internal
> define? Ex.:
Absolutely not.