Re: [dpdk-dev] [PATCH v8 06/10] ipsec: add transmit segmentation offload support

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> > +/* check if packet will exceed MSS and segmentation is required */
> > +static inline int
> > +esn_outb_nb_segments(struct rte_mbuf *m) {
> 
> DPDK codying style pls.
> 
> > +	uint16_t segments = 1;
> > +	uint16_t pkt_l3len = m->pkt_len - m->l2_len;
> > +
> > +	/* Only support segmentation for UDP/TCP flows */
> > +	if (!(m->packet_type & (RTE_PTYPE_L4_UDP | RTE_PTYPE_L4_TCP)))
> 
> For ptypes it is not a bit flag, it should be something like:
> 
> pt =  m->packet_type & RTE_PTYPE_L4_MASK;
> if (pt == RTE_PTYPE_L4_UDP || pt == RTE_PTYPE_L4_TCP) {...}
> 
> BTW, ptype is usually used for RX path.
> If you expect user to setup it up on TX path - it has to be documented in formal API comments.

Thinking a bit more about it:
Do we really need to force user to set ptypes to use this feature?
Might be something as simple as follows would work:

1. If user expects that he would need TSO for the ESP packet,
he would simply set PKT_TX_TCP_SEG flag and usual  offload fields required
(l2_len, l3_len, l4_len, tso_segsz).
2. In ipsec lib we'll check for PKT_TX_TCP_SEG - and if it is set we'll do extra processing
(as your patch does - calc number of segments, fill ESP data in a bit different way,
fill outer_l2_len, outer_l3_len etc.)
3. If user overestimate things and there would be just one segment within packet with
PKT_TX_TCP_SEG - I don't think it is a big deal, things will keep working correctly and AFAIK
there would be no slowdown.
 
That way it should probably simplify things for this feature and would help
avoid setting extra ol_flags inside ipsec lib. 
One side question - how PMD will report that this feature is supported?
Would it be extra field in rte_security_ipsec_xform or something different? 

> 
> > +		return segments;
> > +
> > +	if (m->tso_segsz > 0 && pkt_l3len > m->tso_segsz) {
> > +		segments = pkt_l3len / m->tso_segsz;
> > +		if (segments * m->tso_segsz < pkt_l3len)
> > +			segments++;
> 
> Why not simply:
> segments =  (pkt_l3len >= m->tso_segsz) ? 1 : (pkt_l3len + m->tso_segsz - 1) / m->tso_segsz;
> ?
> 
> > +		if  (m->packet_type & RTE_PTYPE_L4_TCP)
> > +			m->ol_flags |= (PKT_TX_TCP_SEG | PKT_TX_TCP_CKSUM);
> > +		else
> > +			m->ol_flags |= (PKT_TX_UDP_SEG | PKT_TX_UDP_CKSUM);
> > +	}
> > +
> > +	return segments;
> > +}
> > +