From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f169.google.com (mail-we0-f169.google.com [74.125.82.169]) by dpdk.org (Postfix) with ESMTP id 97A67B3AA for ; Tue, 12 Aug 2014 12:00:55 +0200 (CEST) Received: by mail-we0-f169.google.com with SMTP id u56so9724582wes.28 for ; Tue, 12 Aug 2014 03:03:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=mXBj+tnCkm+gMNZmYNkNfW/Qn+AP+VZHSWtWHYX9nnk=; b=Tc8Ra1i7j66pa9gllEvEk7bmizrRa6ULkKBGqdMIifZn0HfjH1Owzdf22HstBYNCmx 8MipHrDRe9twJpBCCaPo+2HIGRCdZW6itNvHX9NcMpmHp5+bN1DvOlgVa4YIcrp/iujq pZl4fyPLUzE+D8SmcieCRlmU5YquyYU4XCn5oLnfz/C4v81ShjdUywOkvjQSopeisskO 78vzXK05CTRbjESc72jpLMdk9Lc3mW8nMlHojbdu+N6vU7+JAmfVOYpuzSAFGT61Lde3 gSscLKh7k5FOTkOBpU/Mf29D9C8fKMq6SwCyyNC8CVOBoLeQ4irw3sI3ZT40XBqzRQtr 1gfg== X-Gm-Message-State: ALoCoQk/SM/h5G4X9D5FA5pkXyjEOOctY+fYBZkBQ4Ha2vESJGwLeaSA0RMf/of8HevGleaYeYpH X-Received: by 10.194.58.148 with SMTP id r20mr4223761wjq.66.1407837830343; Tue, 12 Aug 2014 03:03:50 -0700 (PDT) Received: from [10.16.0.195] (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by mx.google.com with ESMTPSA id fs3sm54446391wic.20.2014.08.12.03.03.48 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 12 Aug 2014 03:03:49 -0700 (PDT) Message-ID: <53E9E684.1040001@6wind.com> Date: Tue, 12 Aug 2014 12:03:48 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: Bruce Richardson , dev@dpdk.org References: <1407789890-17355-1-git-send-email-bruce.richardson@intel.com> <1407789890-17355-7-git-send-email-bruce.richardson@intel.com> In-Reply-To: <1407789890-17355-7-git-send-email-bruce.richardson@intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC PATCH 06/14] mbuf: reorder fields by time-of-use X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Aug 2014 10:00:55 -0000 Hi Bruce, On 08/11/2014 10:44 PM, Bruce Richardson wrote: > * Reorder the fields in the mbuf so that we have fields that are used > together side-by-side in the structure. This means that we have a > contiguous block of 8-bytes in the mbuf which are used to reset an mbuf > of descriptor rearm. > * Where needed add in a dummy fields to overwrite values 8 or 16 bytes > at a time, when doing RX or RX descriptor rearm. This avoids compiler > warnings when using uint64_t values to overwrite a set of smaller > values. > * At the end, place fields that are only used for TX or for the slower > RX path, and mark them as down to be moved to a second cache line. > > Signed-off-by: Bruce Richardson > --- > lib/librte_mbuf/rte_mbuf.c | 2 +- > lib/librte_mbuf/rte_mbuf.h | 37 +++++++++++++++++++++---------------- > 2 files changed, 22 insertions(+), 17 deletions(-) > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c > index 64f1587..594b910 100644 > --- a/lib/librte_mbuf/rte_mbuf.c > +++ b/lib/librte_mbuf/rte_mbuf.c > @@ -161,7 +161,7 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len) > > fprintf(f, "dump mbuf at 0x%p, phys=%"PRIx64", buf_len=%u\n", > m, (uint64_t)m->buf_physaddr, (unsigned)m->buf_len); > - fprintf(f, " pkt_len=%"PRIu32", ol_flags=%"PRIx16", nb_segs=%u, " > + fprintf(f, " pkt_len=%"PRIu32", ol_flags=%"PRIx64", nb_segs=%u, " > "in_port=%u\n", m->pkt_len, m->ol_flags, > (unsigned)m->nb_segs, (unsigned)m->port); > nb_segs = m->nb_segs; I think this should not go in this patch. Another one "change ol_flags to 64 bits" would be nice. > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > index e0981c9..566bb7e 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -132,22 +132,20 @@ union rte_vlan_macip { > /**< MAC+IP length. */ > #define TX_MACIP_LEN_CMP_MASK (TX_MAC_LEN_CMP_MASK | TX_IP_LEN_CMP_MASK) > > + > /** Garbage here. > * The generic rte_mbuf, containing a packet mbuf. > */ > struct rte_mbuf { > - struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */ > void *buf_addr; /**< Virtual address of segment buffer. */ > phys_addr_t buf_physaddr; /**< Physical address of segment buffer. */ > - uint16_t buf_len; /**< Length of segment buffer. */ > > - /* valid for any segment */ > - struct rte_mbuf *next; /**< Next segment of scattered packet. */ > + /* next 8 bytes are initialised on RX descriptor rearm */ > + uint64_t rearm_data[0]; /**< dummy element so we can get uin64_t ptrs > + * to this part of the mbuf without alias error > + */ > + uint16_t buf_len; /**< Length of segment buffer. */ > uint16_t data_off; > - uint16_t data_len; /**< Amount of data in segment buffer. */ > - uint32_t pkt_len; /**< Total pkt len: sum of all segments. */ What do you think about using an union instead? I'm not sure it's clearer, but in case of: union { uint64_t u64; struct { uint16_t buf_len; uint16_t data_off; ... }; }; > - > -#ifdef RTE_MBUF_REFCNT > /** > * 16-bit Reference counter. > * It should only be accessed using the following functions: > @@ -157,20 +155,23 @@ struct rte_mbuf { > * config option. > */ > union { > +#ifdef RTE_MBUF_REFCNT > rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */ > uint16_t refcnt; /**< Non-atomically accessed refcnt */ > - }; > -#else > - uint16_t refcnt_reserved; /**< Do not use this field */ > #endif > - > - /* these fields are valid for first segment only */ > + uint16_t refcnt_reserved; /**< Do not use this field */ > + }; I think this should go in another cosmetic patch. > uint8_t nb_segs; /**< Number of segments. */ > uint8_t port; /**< Input port. */ > - uint16_t ol_flags; /**< Offload features. */ > - uint16_t reserved; /**< Unused field. Required for padding. */ > > - /* offload features, valid for first segment only */ > + /* remaining bytes are set on RX when pulling packet from descriptor */ > + uint64_t ol_flags; /**< Offload features. */ Should be moved to "change ol_flags to 64 bits". > + > + __m128i rx_descriptor_fields1[0]; /**< dummy field used as marker for > + * writes in a vector driver */ Is it a good idea to have a specific type in the generic mbuf structure? Moreover it seems that later in your patch series it's replaced by something else. Also, the 2nd line of comment mixes tabs and spaces. > > + /* second cache line, fields only used in slow path or on TX */ > + struct rte_mempool *pool; /**< Pool from which mbuf was allocated. */ > + struct rte_mbuf *next; /**< Next segment of scattered packet. */ The comment is wrong, this is not a new cache line (introduced later). Regards, Olivier