Re: [dpdk-dev] [PATCH RFC] mbuf: Adjust TX flags to start at bit 32

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Sep 30, 2014 at 01:06:32PM -0400, Neil Horman wrote:
> On Tue, Sep 30, 2014 at 04:26:02PM +0100, Bruce Richardson wrote:
> > This patch takes the existing TX flags defined for the mbuf and shifts
> > each uniquely defined one left so that additional RX flags can be
> > defined without having RX and TX flags mixed together.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 1c6e115..c9fc4ec 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -86,26 +86,26 @@ extern "C" {
> >  #define PKT_RX_IEEE1588_PTP  0x0200 /**< RX IEEE1588 L2 Ethernet PT Packet. */
> >  #define PKT_RX_IEEE1588_TMST 0x0400 /**< RX IEEE1588 L2/L4 timestamped packet.*/
> >  
> > -#define PKT_TX_VLAN_PKT      0x0800 /**< TX packet is a 802.1q VLAN packet. */
> > -#define PKT_TX_IP_CKSUM      0x1000 /**< IP cksum of TX pkt. computed by NIC. */
> > -#define PKT_TX_IPV4_CSUM     0x1000 /**< Alias of PKT_TX_IP_CKSUM. */
> > -#define PKT_TX_IPV4          PKT_RX_IPV4_HDR /**< IPv4 with no IP checksum offload. */
> > -#define PKT_TX_IPV6          PKT_RX_IPV6_HDR /**< IPv6 packet */
> > +#define PKT_TX_VLAN_PKT  (0x0001 << 32) /**< TX packet is a 802.1q VLAN packet. */
> > +#define PKT_TX_IP_CKSUM  (0x0002 << 32) /**< IP cksum of TX pkt. computed by NIC. */
> > +#define PKT_TX_IPV4_CSUM PKT_TX_IP_CKSUM /**< Alias of PKT_TX_IP_CKSUM. */
> > +#define PKT_TX_IPV4      PKT_RX_IPV4_HDR /**< IPv4 with no IP checksum offload. */
> > +#define PKT_TX_IPV6      PKT_RX_IPV6_HDR /**< IPv6 packet */
> >  
> >  /*
> > - * Bit 14~13 used for L4 packet type with checksum enabled.
> > + * Bit 35~34 used for L4 packet type with checksum enabled.
> >   *     00: Reserved
> >   *     01: TCP checksum
> >   *     10: SCTP checksum
> >   *     11: UDP checksum
> >   */
> > -#define PKT_TX_L4_MASK       0x6000 /**< Mask bits for L4 checksum offload request. */
> > -#define PKT_TX_L4_NO_CKSUM   0x0000 /**< Disable L4 cksum of TX pkt. */
> > -#define PKT_TX_TCP_CKSUM     0x2000 /**< TCP cksum of TX pkt. computed by NIC. */
> > -#define PKT_TX_SCTP_CKSUM    0x4000 /**< SCTP cksum of TX pkt. computed by NIC. */
> > -#define PKT_TX_UDP_CKSUM     0x6000 /**< UDP cksum of TX pkt. computed by NIC. */
> > -/* Bit 15 */
> > -#define PKT_TX_IEEE1588_TMST 0x8000 /**< TX IEEE1588 packet to timestamp. */
> > +#define PKT_TX_L4_NO_CKSUM   (0x0000 << 32) /**< Disable L4 cksum of TX pkt. */
> > +#define PKT_TX_TCP_CKSUM     (0x0004 << 32) /**< TCP cksum of TX pkt. computed by NIC. */
> > +#define PKT_TX_SCTP_CKSUM    (0x0008 << 32) /**< SCTP cksum of TX pkt. computed by NIC. */
> > +#define PKT_TX_UDP_CKSUM     (0x000C << 32) /**< UDP cksum of TX pkt. computed by NIC. */
> > +#define PKT_TX_L4_MASK       (0x000C << 32) /**< Mask for L4 cksum offload request. */
> > +/* Bit 36 */
> > +#define PKT_TX_IEEE1588_TMST (0x0010 << 32) /**< TX IEEE1588 packet to timestamp. */
> >  
> >  /* Use final bit of flags to indicate a control mbuf */
> >  #define CTRL_MBUF_FLAG       (1ULL << 63)
> > -- 
> > 1.9.3
> > 
> > 
> 
> I'm not opposed to the patch at all, but I would like to point out that this is
> the sort of change that breaks ABI very easily (which is fine right now given
> the mbuf changes already staged for the release, but still something to be aware
> of).  As such, are there advantages to this patch (other than the niceness of
> human readability)?
> 
> If we're going to reshuffle these flags now, it might be nice to start tx flags
> at the most significant bit and count back, and start rx flags at the least
> significant bit and count up.  That would ensure that we don't reserve flags for
> a direction without need.
> 
> Best
> Neil
>
Good idea, though currently the most significant bit is being used as a flag 
to indicate a control mbuf. My thinking is that we may potentially need 
other flags that are not just for RX or TX, and to have those at the end.  
However, given that that is likely to be the exception case, perhaps we 
could reserve the last byte for non-RX/TX flags and then implement the 
scheme you suggest. What do you think?

/Bruce