Re: [ovs-dev] [PATCH v3 2/2] userspace: Add Generic Segmentation Offloading.
On Tue, Jul 4, 2023 at 9:00 PM Ilya Maximets wrote:
>
> On 6/21/23 22:36, Mike Pattrick wrote:
> > From: Flavio Leitner
> >
> > This provides a software implementation in the case
> > the egress netdev doesn't support segmentation in hardware.
> >
> > The challenge here is to guarantee packet ordering in the
> > original batch that may be full of TSO packets. Each TSO
> > packet can go up to ~64kB, so with segment size of 1440
> > that means about 44 packets for each TSO. Each batch has
> > 32 packets, so the total batch amounts to 1408 normal
> > packets.
> >
> > The segmentation estimates the total number of packets
> > and then the total number of batches. Then allocate
> > enough memory and finally do the work.
> >
> > Finally each batch is sent in order to the netdev.
> >
> > Signed-off-by: Flavio Leitner
> > Co-authored-by: Mike Pattrick
> > Signed-off-by: Mike Pattrick
> > ---
> > lib/automake.mk | 2 +
> > lib/dp-packet-gso.c | 172
> > lib/dp-packet-gso.h | 24 +++
> > lib/dp-packet.h | 11 +++
> > lib/netdev-dpdk.c | 46
> > lib/netdev-linux.c | 58 ---
> > lib/netdev.c| 120 ++-
> > lib/packets.c | 4 +-
> > 8 files changed, 314 insertions(+), 123 deletions(-)
> > create mode 100644 lib/dp-packet-gso.c
> > create mode 100644 lib/dp-packet-gso.h
> >
> > diff --git a/lib/automake.mk b/lib/automake.mk
> > index e64ee76ce..49a92958d 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> > lib/dpctl.h \
> > lib/dp-packet.h \
> > lib/dp-packet.c \
> > + lib/dp-packet-gso.c \
> > + lib/dp-packet-gso.h \
> > lib/dpdk.h \
> > lib/dpif-netdev-extract-study.c \
> > lib/dpif-netdev-lookup.h \
> > diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> > new file mode 100644
> > index 0..bc72e2f90
> > --- /dev/null
> > +++ b/lib/dp-packet-gso.c
> > @@ -0,0 +1,172 @@
> > +/*
> > + * Copyright (c) 2021 Red Hat, Inc.
>
> Need to adjust the year. :)
>
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "coverage.h"
> > +#include "dp-packet.h"
> > +#include "dp-packet-gso.h"
> > +#include "netdev-provider.h"
> > +#include "openvswitch/vlog.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
> > +
> > +COVERAGE_DEFINE(soft_seg_good);
> > +
> > +/* Retuns a new packet that is a segment of packet 'p'.
> > + *
> > + * The new packet is initialized with 'hdr_len' bytes from the
> > + * start of packet 'p' and then appended with 'data_len' bytes
> > + * from the 'data' buffer.
> > + *
> > + * Note: The packet headers are not updated. */
> > +static struct dp_packet *
> > +dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
> > + const char *data, size_t data_len)
> > +{
> > +struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
> > +
> > dp_packet_headroom(p));
> > +
> > +/* Append the original packet headers and then the payload. */
> > +dp_packet_put(seg, dp_packet_data(p), hdr_len);
> > +dp_packet_put(seg, data, data_len);
> > +
> > +/* The new segment should have the same offsets. */
> > +seg->l2_5_ofs = p->l2_5_ofs;
> > +seg->l3_ofs = p->l3_ofs;
> > +seg->l4_ofs = p->l4_ofs;
> > +
> > +/* The protocol headers remain the same, so preserve hash and mark. */
> > +*dp_packet_rss_ptr(seg) = dp_packet_get_rss_hash(p);
> > +*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
> > +
> > +/* The segment should inherit all the offloading flags from the
> > + * original packet, except for the TCP segmentation, external
> > + * buffer and indirect buffer flags. */
> > +*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
> > +& ~(DP_PACKET_OL_TX_TCP_SEG | DP_PACKET_OL_EXTERNAL
> > +| DP_PACKET_OL_INDIRECT);
>
> We should inherit DP_PACKET_OL_SUPPORTED_MASK & ~DP_PACKET_OL_TX_TCP_SEG.
> So,
>
> *dp_packet_ol_flags_ptr(seg) &= DP_PACKET_OL_SUPPORTED_MASK
> & ~DP_PACKET_OL_TX_TCP_SEG;
>
> Is that right?
>
> > +
> > +dp_packet_hwol_reset_tcp_seg(seg);
>
> Also, this function just resets the
Re: [ovs-dev] [PATCH v3 2/2] userspace: Add Generic Segmentation Offloading.
On 6/21/23 22:36, Mike Pattrick wrote:
> From: Flavio Leitner
>
> This provides a software implementation in the case
> the egress netdev doesn't support segmentation in hardware.
>
> The challenge here is to guarantee packet ordering in the
> original batch that may be full of TSO packets. Each TSO
> packet can go up to ~64kB, so with segment size of 1440
> that means about 44 packets for each TSO. Each batch has
> 32 packets, so the total batch amounts to 1408 normal
> packets.
>
> The segmentation estimates the total number of packets
> and then the total number of batches. Then allocate
> enough memory and finally do the work.
>
> Finally each batch is sent in order to the netdev.
>
> Signed-off-by: Flavio Leitner
> Co-authored-by: Mike Pattrick
> Signed-off-by: Mike Pattrick
> ---
> lib/automake.mk | 2 +
> lib/dp-packet-gso.c | 172
> lib/dp-packet-gso.h | 24 +++
> lib/dp-packet.h | 11 +++
> lib/netdev-dpdk.c | 46
> lib/netdev-linux.c | 58 ---
> lib/netdev.c| 120 ++-
> lib/packets.c | 4 +-
> 8 files changed, 314 insertions(+), 123 deletions(-)
> create mode 100644 lib/dp-packet-gso.c
> create mode 100644 lib/dp-packet-gso.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index e64ee76ce..49a92958d 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -118,6 +118,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/dpctl.h \
> lib/dp-packet.h \
> lib/dp-packet.c \
> + lib/dp-packet-gso.c \
> + lib/dp-packet-gso.h \
> lib/dpdk.h \
> lib/dpif-netdev-extract-study.c \
> lib/dpif-netdev-lookup.h \
> diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> new file mode 100644
> index 0..bc72e2f90
> --- /dev/null
> +++ b/lib/dp-packet-gso.c
> @@ -0,0 +1,172 @@
> +/*
> + * Copyright (c) 2021 Red Hat, Inc.
Need to adjust the year. :)
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include "coverage.h"
> +#include "dp-packet.h"
> +#include "dp-packet-gso.h"
> +#include "netdev-provider.h"
> +#include "openvswitch/vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
> +
> +COVERAGE_DEFINE(soft_seg_good);
> +
> +/* Retuns a new packet that is a segment of packet 'p'.
> + *
> + * The new packet is initialized with 'hdr_len' bytes from the
> + * start of packet 'p' and then appended with 'data_len' bytes
> + * from the 'data' buffer.
> + *
> + * Note: The packet headers are not updated. */
> +static struct dp_packet *
> +dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
> + const char *data, size_t data_len)
> +{
> +struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
> +
> dp_packet_headroom(p));
> +
> +/* Append the original packet headers and then the payload. */
> +dp_packet_put(seg, dp_packet_data(p), hdr_len);
> +dp_packet_put(seg, data, data_len);
> +
> +/* The new segment should have the same offsets. */
> +seg->l2_5_ofs = p->l2_5_ofs;
> +seg->l3_ofs = p->l3_ofs;
> +seg->l4_ofs = p->l4_ofs;
> +
> +/* The protocol headers remain the same, so preserve hash and mark. */
> +*dp_packet_rss_ptr(seg) = dp_packet_get_rss_hash(p);
> +*dp_packet_flow_mark_ptr(seg) = *dp_packet_flow_mark_ptr(p);
> +
> +/* The segment should inherit all the offloading flags from the
> + * original packet, except for the TCP segmentation, external
> + * buffer and indirect buffer flags. */
> +*dp_packet_ol_flags_ptr(seg) = *dp_packet_ol_flags_ptr(p)
> +& ~(DP_PACKET_OL_TX_TCP_SEG | DP_PACKET_OL_EXTERNAL
> +| DP_PACKET_OL_INDIRECT);
We should inherit DP_PACKET_OL_SUPPORTED_MASK & ~DP_PACKET_OL_TX_TCP_SEG.
So,
*dp_packet_ol_flags_ptr(seg) &= DP_PACKET_OL_SUPPORTED_MASK
& ~DP_PACKET_OL_TX_TCP_SEG;
Is that right?
> +
> +dp_packet_hwol_reset_tcp_seg(seg);
Also, this function just resets the DP_PACKET_OL_TX_TCP_SEG.
So, above should be just:
*dp_packet_ol_flags_ptr(seg) &= DP_PACKET_OL_SUPPORTED_MASK;
The same as in the clone function. Either way one of these operations
is redundant.
> +
> +return seg;
> +}
> +
> +/* Returns the calculated number of TCP segments in packet 'p'. */
> +int
>
