Hi Konstantin,
On Thu, Sep 14, 2017 at 06:10:37AM +0800, Ananyev, Konstantin wrote:
>
> Hi Jiayu,
>
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ananyev, Konstantin
> > > > Sent: Tuesday, September 12, 2017 12:18 PM
> > > > To: Hu, Jiayu <jiayu...@intel.com>; dev@dpdk.org
> > > > Cc: Kavanagh, Mark B <mark.b.kavan...@intel.com>; Tan, Jianfeng
> > > > <jianfeng....@intel.com>
> > > > Subject: RE: [PATCH v3 2/5] gso: add TCP/IPv4 GSO support
> > > >
> > > > > result, when all of its GSOed segments are freed, the packet is freed
> > > > > automatically.
> > > > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c
> > > > > index dda50ee..95f6ea6 100644
> > > > > --- a/lib/librte_gso/rte_gso.c
> > > > > +++ b/lib/librte_gso/rte_gso.c
> > > > > @@ -33,18 +33,53 @@
> > > > >
> > > > > #include <errno.h>
> > > > >
> > > > > +#include <rte_log.h>
> > > > > +
> > > > > #include "rte_gso.h"
> > > > > +#include "gso_common.h"
> > > > > +#include "gso_tcp4.h"
> > > > >
> > > > > int
> > > > > rte_gso_segment(struct rte_mbuf *pkt,
> > > > > - struct rte_gso_ctx gso_ctx __rte_unused,
> > > > > + struct rte_gso_ctx gso_ctx,
> > > > > struct rte_mbuf **pkts_out,
> > > > > uint16_t nb_pkts_out)
> > > > > {
> > > > > + struct rte_mempool *direct_pool, *indirect_pool;
> > > > > + struct rte_mbuf *pkt_seg;
> > > > > + uint16_t gso_size;
> > > > > + uint8_t ipid_delta;
> > > > > + int ret = 1;
> > > > > +
> > > > > if (pkt == NULL || pkts_out == NULL || nb_pkts_out < 1)
> > > > > return -EINVAL;
> > > > >
> > > > > - pkts_out[0] = pkt;
> > > > > + if (gso_ctx.gso_size >= pkt->pkt_len ||
> > > > > + (pkt->packet_type & gso_ctx.gso_types) !=
> > > > > + pkt->packet_type) {
> > > > > + pkts_out[0] = pkt;
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + direct_pool = gso_ctx.direct_pool;
> > > > > + indirect_pool = gso_ctx.indirect_pool;
> > > > > + gso_size = gso_ctx.gso_size;
> > > > > + ipid_delta = gso_ctx.ipid_flag == RTE_GSO_IPID_INCREASE;
> > > > > +
> > > > > + if (is_ipv4_tcp(pkt->packet_type)) {
> > > >
> > > > Probably we need here:
> > > > If (is_ipv4_tcp(pkt->packet_type) && (gso_ctx->gso_types &
> > > > DEV_TX_OFFLOAD_TCP_TSO) != 0) {...
> > >
> > > Sorry, actually it probably should be:
> > > If (pkt->ol_flags & (PKT_TX_TCP_SEG | PKT_TX_IPV4) == PKT_TX_IPV4 &&
> > > (gso_ctx->gso_types & DEV_TX_OFFLOAD_TCP_TSO) != 0) {...
> >
> > I don't quite understand why the GSO library should be aware if the TSO
> > flag is set or not. Applications can query device TSO capability before
> > they call the GSO library. Do I misundertsand anything?
> >
> > Additionally, we don't need to check if the packet is a TCP/IPv4 packet
> > here?
>
> Well, right now PMD we doesn't rely on ptype to figure out what type of
> packet and
> what TX offload have to be performed.
> Instead it looks at TX part of ol_flags, and
> My thought was that as what we doing is actually TSO in SW, it would be good
> to use the same API here too.
> Also with that approach, by setting ol_flags properly user can use the same
> gso_ctx and still
> specify what segmentation to perform on a per-packet basis.
>
> Alternative way is to rely on ptype to distinguish should segmentation be
> performed on that package or not.
> The only advantage I see here is that if someone would like to add GSO for
> some new protocol,
> he wouldn't need to introduce new TX flag value for mbuf.ol_flags.
> Though he still would need to update TX_OFFLOAD_* capabilities and probably
> packet_type definitions.
>
> So from my perspective first variant (use HW TSO API) is more plausible.
> Wonder what is your and Mark opinions here?
In the first choice, you mean:
the GSO library uses gso_ctx->gso_types and mbuf->ol_flags to call a specific
GSO
segmentation function (e.g. gso_tcp4_segment(), gso_tunnel_xxx()) for each
input packet.
Applications should parse the packet type, and set an exactly correct
DEV_TX_OFFLOAD_*_TSO
flag to gso_types and ol_flags according to the packet type. That is, the value
of gso_types
is on a per-packet basis. Using gso_ctx->gso_types and mbuf->ol_flags at the
same time
is because that DEV_TX_OFFLOAD_*_TSO only tells tunnelling type and the inner
L4 type, and
we need to know L3 type by ol_flags. With this design, HW segmentation and SW
segmentation
are indeed consistent.
If I understand it correctly, applications need to set 'ol_flags = PKT_TX_IPV4'
and
'gso_types = DEV_TX_OFFLOAD_VXLAN_TNL_TSO' for a
"ether+ipv4+udp+vxlan+ether+ipv4+
tcp+payload" packet. But PKT_TX_IPV4 just present the inner L3 type for
tunneled packet.
How about the outer L3 type? Always assume the inner and the outer L3 type are
the same?
Jiayu
> Konstantin
