Re: [dpdk-dev] [PATCH v3 1/3] ethdev: introduce conntrack flow action and item

[Bing Zhao <bingz@nvidia.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, April 19, 2021 10:06 PM
> To: Bing Zhao <bingz@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; ferruh.yigit@intel.com;
> andrew.rybchenko@oktetlabs.ru; dev@dpdk.org;
> ajit.khaparde@broadcom.com; xiaoyun.li@intel.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/3] ethdev: introduce conntrack
> flow action and item
> 
> External email: Use caution opening links or attachments
> 
> 
> 16/04/2021 19:54, Bing Zhao:
> > +/**
> > + * The packet is valid after conntrack checking.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_VALID RTE_BIT32(0)
> > +/**
> > + * The state of the connection is changed.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_CHANGED RTE_BIT32(1)
> > +/**
> > + * Error is detected on this packet for this connection and
> > + * an invalid state is set.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_INVALID RTE_BIT32(2)
> > +/**
> > + * The HW connection tracking module is disabled.
> > + * It can be due to application command or an invalid state.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_DISABLED RTE_BIT32(3)
> > +/**
> > + * The packet contains some bad field(s) and cannot continue
> > + * with the conntrack module checking.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_BAD RTE_BIT32(4)
> 
> I like it better now that all bits have the same prefix, thanks.
> 
> > +enum rte_flow_conntrack_state {
> > +     /**< SYN-ACK packet was seen. */
> > +     RTE_FLOW_CONNTRACK_STATE_SYN_RECV,
> > +     /**< 3-way handshake was done. */
> > +     RTE_FLOW_CONNTRACK_STATE_ESTABLISHED,
> > +     /**< First FIN packet was received to close the connection.
> */
> > +     RTE_FLOW_CONNTRACK_STATE_FIN_WAIT,
> > +     /**< First FIN was ACKed. */
> > +     RTE_FLOW_CONNTRACK_STATE_CLOSE_WAIT,
> > +     /**< Second FIN was received, waiting for the last ACK. */
> > +     RTE_FLOW_CONNTRACK_STATE_LAST_ACK,
> > +     /**< Second FIN was ACKed, connection was closed. */
> > +     RTE_FLOW_CONNTRACK_STATE_TIME_WAIT,
> > +};
> 
> These doxygen comments should not have "<" in them, because they are
> "before".

All "<" are removed, thanks.

> 
> [...]
> > +     /** No TCP check will be done except the state change. */
> > +     uint32_t liberal_mode:1;
> > +     /**<The current state of this connection. */
> 
> s,/**<,/** ,
> 
> > +     enum rte_flow_conntrack_state state;
> 
> Looks good overrall, thanks.
> 

BR. Bing