[EXT] Re: [PATCH v3 1/4] ethdev: introduce IP reassembly offload

[Akhil Goyal]

> > +/* Flag to offload IP reassembly for IPv4 packets. */
> > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV4 (RTE_BIT32(0))
> > +/* Flag to offload IP reassembly for IPv6 packets. */
> > +#define RTE_ETH_DEV_REASSEMBLY_F_IPV6 (RTE_BIT32(1))
> > +/**
> > + * A structure used to get/set IP reassembly configuration.
> > + *
> > + * If RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY flag is set in offloads field,
> > + * the PMD will attempt IP reassembly for the received packets as per
> > + * properties defined in this structure.
> > + */
> > +struct rte_eth_ip_reass_params {
> 
> As a generic comment, what do you think to use full 'reassembly' instead
> of 'reass' short version, to clarify/simplify the meaning?

Full reassembly was used in most places. But here the struct name would be too big.
IMO, reass is good enough here. Though, no strong opinion. Will change if you insist.

> 
> > +	/** Maximum time in ms which PMD can wait for other fragments. */
> > +	uint32_t reass_timeout_ms;
> > +	/** Maximum number of fragments that can be reassembled. */
> > +	uint16_t max_frags;
> > +	/**
> > +	 * Flags to enable reassembly of packet types -
> > +	 * RTE_ETH_DEV_REASSEMBLY_F_xxx.
> > +	 */
> > +	uint16_t flags;
> > +};
> > +
> >   /**
> >    * A structure used to retrieve the contextual information of
> >    * an Ethernet device, such as the controlling driver of the
> > @@ -1841,8 +1865,10 @@ struct rte_eth_dev_info {
> >   	 * embedded managed interconnect/switch.
> >   	 */
> >   	struct rte_eth_switch_info switch_info;
> > +	/** IP reassembly offload capabilities that a device can support. */
> > +	struct rte_eth_ip_reass_params reass_capa;
> >
> 
> "struct rte_eth_dev_info" & 'rte_eth_dev_info_get()' are very common,
> all applications that use net devices and even some internal APIs rely on
> this struct and API.
> It makes me uneasy to extend this struct with rarely used features,
> worrying on loading to much (capability/status/config) on single
> API/struct can cause an unmaintainable code by time.
> 
> Also most of the time (if not always) offload flag is just an on/off flag
> to the PMD, application set/unset offload flag and PMD knows what to do.
> But for this case some capability variables, and a configuration API is
> required/involved.
> 
> For considering above two cases, what do you think implement this as
> control plane APIs instead of offload flag?
> There are already 'conf_set()' and 'conf_get()' APIs introduced in coming
> patches, introducing an additional 'capability_get()' API removes the need
> of change in "struct rte_eth_dev_info" and
> 'RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY'
> can be removed.
> Thomas, Andrew, what do you think?

We are ok to add a new dev_op for capability_get() if we agree on that.
Thomas, Andrew, let me know if you think otherwise.

> 
> 
> > -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> > +	uint64_t reserved_64s[1]; /**< Reserved for future fields */
> >   	void *reserved_ptrs[2];   /**< Reserved for future fields */
> >   };
> >