Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Thu, 10 Feb 2022, Jakub Kicinski wrote: > On Thu, 10 Feb 2022 10:53:24 +0200 Paul Blakey wrote: > > > The calls seem a little heavy for single byte replacements. > > > Can you instead add a helper based on csum_replace4() maybe? > > > > > > BTW doesn't pedit have the same problem? > > > > I don't think they are heavier then csum_replace4, > > csum_replace4 is a handful of instructions all of which will be inlined. > csum_partial() is a function call and handles variable lengths. > > > but they are more bulletproof in my opinion, since they handle both > > the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) > > Yes, that's why I said "add a helper based on", a skb helper which > checks the csum type of the packet but instead of calling csum_partial > for no reason does the adjustment directly. Then sure, I will do that and send v2. > > > and resemble what editing of the packet should have done - pull the > > header, edit, and then push it back. > > That's not what this code is doing, so the argument does not stand IMO. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Thu, 10 Feb 2022 10:53:24 +0200 Paul Blakey wrote: > > The calls seem a little heavy for single byte replacements. > > Can you instead add a helper based on csum_replace4() maybe? > > > > BTW doesn't pedit have the same problem? > > I don't think they are heavier then csum_replace4, csum_replace4 is a handful of instructions all of which will be inlined. csum_partial() is a function call and handles variable lengths. > but they are more bulletproof in my opinion, since they handle both > the COMPLETE and PARTIAL csum cases (in __skb_postpull_rcsum()) Yes, that's why I said "add a helper based on", a skb helper which checks the csum type of the packet but instead of calling csum_partial for no reason does the adjustment directly. > and resemble what editing of the packet should have done - pull the > header, edit, and then push it back. That's not what this code is doing, so the argument does not stand IMO. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Wed, 9 Feb 2022, Jakub Kicinski wrote:
> On Mon, 7 Feb 2022 16:41:01 +0200 Paul Blakey wrote:
> > Ipv6 ttl, label and tos fields are modified without first
> > pulling/pushing the ipv6 header, which would have updated
> > the hw csum (if available). This might cause csum validation
> > when sending the packet to the stack, as can be seen in
> > the trace below.
> >
> > Fix this by calling postpush/postpull checksum calculation
> > which will update the hw csum if needed.
>
> > -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask)
> > +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8
> > ipv6_tclass, __u8 mask)
> > {
> > + skb_postpull_rcsum(skb, nh, 4);
> > +
> > + ipv6_change_dsfield(nh, ~mask, ipv6_tclass);
> > +
> > + skb_postpush_rcsum(skb, nh, 4);
> > +}
>
> The calls seem a little heavy for single byte replacements.
> Can you instead add a helper based on csum_replace4() maybe?
>
> BTW doesn't pedit have the same problem?
>
I don't think they are heavier then csum_replace4, but they are more
bulletproof in my opinion, since they handle both the COMPLETE and PARTIAL
csum cases (in __skb_postpull_rcsum()) and resemble what editing of the
packet should have done - pull the header, edit, and then push it back.
RE pedit, not really the issue here, but i guess pedit should be followed
by act_csum with the relevant checksum fields (as we do when offloading
ovs set() action to tc pedit + csum actions).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net 1/1] openvswitch: Fix setting ipv6 fields causing hw csum failure
On Mon, 7 Feb 2022 16:41:01 +0200 Paul Blakey wrote:
> Ipv6 ttl, label and tos fields are modified without first
> pulling/pushing the ipv6 header, which would have updated
> the hw csum (if available). This might cause csum validation
> when sending the packet to the stack, as can be seen in
> the trace below.
>
> Fix this by calling postpush/postpull checksum calculation
> which will update the hw csum if needed.
> -static void set_ipv6_fl(struct ipv6hdr *nh, u32 fl, u32 mask)
> +static void set_ipv6_dsfield(struct sk_buff *skb, struct ipv6hdr *nh, __u8
> ipv6_tclass, __u8 mask)
> {
> + skb_postpull_rcsum(skb, nh, 4);
> +
> + ipv6_change_dsfield(nh, ~mask, ipv6_tclass);
> +
> + skb_postpush_rcsum(skb, nh, 4);
> +}
The calls seem a little heavy for single byte replacements.
Can you instead add a helper based on csum_replace4() maybe?
BTW doesn't pedit have the same problem?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
