Patch Comment List
List patch comments
GET /api/patches/142/comments/?format=api
[ { "id": 352, "web_url": "https://patches.dpdk.org/comment/352/", "msgid": "<53E9E684.1040001@6wind.com>", "list_archive_url": "https://inbox.dpdk.org/dev/53E9E684.1040001@6wind.com", "date": "2014-08-12T10:03:48", "subject": "Re: [dpdk-dev] [RFC PATCH 06/14] mbuf: reorder fields by time-of-use", "submitter": { "id": 8, "url": "https://patches.dpdk.org/api/people/8/?format=api", "name": "Olivier Matz", "email": "olivier.matz@6wind.com" }, "content": "Hi Bruce,\n\nOn 08/11/2014 10:44 PM, Bruce Richardson wrote:\n> * Reorder the fields in the mbuf so that we have fields that are used\n> together side-by-side in the structure. This means that we have a\n> contiguous block of 8-bytes in the mbuf which are used to reset an mbuf\n> of descriptor rearm.\n> * Where needed add in a dummy fields to overwrite values 8 or 16 bytes\n> at a time, when doing RX or RX descriptor rearm. This avoids compiler\n> warnings when using uint64_t values to overwrite a set of smaller\n> values.\n> * At the end, place fields that are only used for TX or for the slower\n> RX path, and mark them as down to be moved to a second cache line.\n>\n> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>\n> ---\n> lib/librte_mbuf/rte_mbuf.c | 2 +-\n> lib/librte_mbuf/rte_mbuf.h | 37 +++++++++++++++++++++----------------\n> 2 files changed, 22 insertions(+), 17 deletions(-)\n>\n> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c\n> index 64f1587..594b910 100644\n> --- a/lib/librte_mbuf/rte_mbuf.c\n> +++ b/lib/librte_mbuf/rte_mbuf.c\n> @@ -161,7 +161,7 @@ rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)\n>\n> \tfprintf(f, \"dump mbuf at 0x%p, phys=%\"PRIx64\", buf_len=%u\\n\",\n> \t m, (uint64_t)m->buf_physaddr, (unsigned)m->buf_len);\n> -\tfprintf(f, \" pkt_len=%\"PRIu32\", ol_flags=%\"PRIx16\", nb_segs=%u, \"\n> +\tfprintf(f, \" pkt_len=%\"PRIu32\", ol_flags=%\"PRIx64\", nb_segs=%u, \"\n> \t \"in_port=%u\\n\", m->pkt_len, m->ol_flags,\n> \t (unsigned)m->nb_segs, (unsigned)m->port);\n> \tnb_segs = m->nb_segs;\n\nI think this should not go in this patch. Another one \"change ol_flags\nto 64 bits\" would be nice.\n\n\n> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h\n> index e0981c9..566bb7e 100644\n> --- a/lib/librte_mbuf/rte_mbuf.h\n> +++ b/lib/librte_mbuf/rte_mbuf.h\n> @@ -132,22 +132,20 @@ union rte_vlan_macip {\n> /**< MAC+IP length. */\n> #define TX_MACIP_LEN_CMP_MASK (TX_MAC_LEN_CMP_MASK | TX_IP_LEN_CMP_MASK)\n>\n> +\n> /**\n\nGarbage here.\n\n> * The generic rte_mbuf, containing a packet mbuf.\n> */\n> struct rte_mbuf {\n> -\tstruct rte_mempool *pool; /**< Pool from which mbuf was allocated. */\n> \tvoid *buf_addr; /**< Virtual address of segment buffer. */\n> \tphys_addr_t buf_physaddr; /**< Physical address of segment buffer. */\n> -\tuint16_t buf_len; /**< Length of segment buffer. */\n>\n> -\t/* valid for any segment */\n> -\tstruct rte_mbuf *next; /**< Next segment of scattered packet. */\n> +\t/* next 8 bytes are initialised on RX descriptor rearm */\n> +\tuint64_t rearm_data[0]; /**< dummy element so we can get uin64_t ptrs\n> +\t * to this part of the mbuf without alias error\n> +\t */\n> +\tuint16_t buf_len; /**< Length of segment buffer. */\n> \tuint16_t data_off;\n> -\tuint16_t data_len; /**< Amount of data in segment buffer. */\n> -\tuint32_t pkt_len; /**< Total pkt len: sum of all segments. */\n\nWhat do you think about using an union instead? I'm not sure it's\nclearer, but in case of:\n\nunion {\n\tuint64_t u64;\n\tstruct {\n\t\tuint16_t buf_len;\n\t\tuint16_t data_off;\n\t\t...\n\t};\n};\n\n\n> -\n> -#ifdef RTE_MBUF_REFCNT\n> \t/**\n> \t * 16-bit Reference counter.\n> \t * It should only be accessed using the following functions:\n> @@ -157,20 +155,23 @@ struct rte_mbuf {\n> \t * config option.\n> \t */\n> \tunion {\n> +#ifdef RTE_MBUF_REFCNT\n> \t\trte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */\n> \t\tuint16_t refcnt; /**< Non-atomically accessed refcnt */\n> -\t};\n> -#else\n> -\tuint16_t refcnt_reserved; /**< Do not use this field */\n> #endif\n> -\n> -\t/* these fields are valid for first segment only */\n> +\t\tuint16_t refcnt_reserved; /**< Do not use this field */\n> +\t};\n\nI think this should go in another cosmetic patch.\n\n\n> \tuint8_t nb_segs; /**< Number of segments. */\n> \tuint8_t port; /**< Input port. */\n> -\tuint16_t ol_flags; /**< Offload features. */\n> -\tuint16_t reserved; /**< Unused field. Required for padding. */\n>\n> -\t/* offload features, valid for first segment only */\n> +\t/* remaining bytes are set on RX when pulling packet from descriptor */\n> +\tuint64_t ol_flags; /**< Offload features. */\n\nShould be moved to \"change ol_flags to 64 bits\".\n\n> +\n> +\t__m128i rx_descriptor_fields1[0]; /**< dummy field used as marker for\n> +\t * writes in a vector driver */\n\nIs it a good idea to have a specific type in the generic mbuf structure?\nMoreover it seems that later in your patch series it's replaced by\nsomething else.\n\nAlso, the 2nd line of comment mixes tabs and spaces.\n\n>\n> +\t/* second cache line, fields only used in slow path or on TX */\n> +\tstruct rte_mempool *pool; /**< Pool from which mbuf was allocated. */\n> +\tstruct rte_mbuf *next; /**< Next segment of scattered packet. */\n\nThe comment is wrong, this is not a new cache line (introduced later).\n\n\n\nRegards,\nOlivier", "headers": { "Return-Path": "<olivier.matz@6wind.com>", "Received": [ "from mail-we0-f169.google.com (mail-we0-f169.google.com\n\t[74.125.82.169]) by dpdk.org (Postfix) with ESMTP id 97A67B3AA\n\tfor <dev@dpdk.org>; Tue, 12 Aug 2014 12:00:55 +0200 (CEST)", "by mail-we0-f169.google.com with SMTP id u56so9724582wes.28\n\tfor <dev@dpdk.org>; Tue, 12 Aug 2014 03:03:50 -0700 (PDT)", "from [10.16.0.195] (guy78-3-82-239-227-177.fbx.proxad.net.\n\t[82.239.227.177]) by mx.google.com with ESMTPSA id\n\tfs3sm54446391wic.20.2014.08.12.03.03.48 for <multiple recipients>\n\t(version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128);\n\tTue, 12 Aug 2014 03:03:49 -0700 (PDT)" ], "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20130820;\n\th=x-gm-message-state:message-id:date:from:user-agent:mime-version:to\n\t:subject:references:in-reply-to:content-type\n\t:content-transfer-encoding;\n\tbh=mXBj+tnCkm+gMNZmYNkNfW/Qn+AP+VZHSWtWHYX9nnk=;\n\tb=Tc8Ra1i7j66pa9gllEvEk7bmizrRa6ULkKBGqdMIifZn0HfjH1Owzdf22HstBYNCmx\n\t8MipHrDRe9twJpBCCaPo+2HIGRCdZW6itNvHX9NcMpmHp5+bN1DvOlgVa4YIcrp/iujq\n\tpZl4fyPLUzE+D8SmcieCRlmU5YquyYU4XCn5oLnfz/C4v81ShjdUywOkvjQSopeisskO\n\t78vzXK05CTRbjESc72jpLMdk9Lc3mW8nMlHojbdu+N6vU7+JAmfVOYpuzSAFGT61Lde3\n\tgSscLKh7k5FOTkOBpU/Mf29D9C8fKMq6SwCyyNC8CVOBoLeQ4irw3sI3ZT40XBqzRQtr\n\t1gfg==", "X-Gm-Message-State": "ALoCoQk/SM/h5G4X9D5FA5pkXyjEOOctY+fYBZkBQ4Ha2vESJGwLeaSA0RMf/of8HevGleaYeYpH", "X-Received": "by 10.194.58.148 with SMTP id r20mr4223761wjq.66.1407837830343; \n\tTue, 12 Aug 2014 03:03:50 -0700 (PDT)", "Message-ID": "<53E9E684.1040001@6wind.com>", "Date": "Tue, 12 Aug 2014 12:03:48 +0200", "From": "Olivier MATZ <olivier.matz@6wind.com>", "User-Agent": "Mozilla/5.0 (X11; Linux x86_64;\n\trv:24.0) Gecko/20100101 Icedove/24.5.0", "MIME-Version": "1.0", "To": "Bruce Richardson <bruce.richardson@intel.com>, dev@dpdk.org", "References": "<1407789890-17355-1-git-send-email-bruce.richardson@intel.com>\n\t<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 <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Tue, 12 Aug 2014 10:00:55 -0000" }, "addressed": null } ]