Patch Comment Detail
get:
Show a patch comment.
patch:
Update a patch comment.
put:
Update a patch comment.
GET /api/patches/139675/comments/169673/?format=api
{ "id": 169673, "web_url": "https://patches.dpdk.org/comment/169673/", "msgid": "<98CBD80474FA8B44BF855DF32C47DC35E9F3EF@smartserver.smartshare.dk>", "list_archive_url": "https://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35E9F3EF@smartserver.smartshare.dk", "date": "2024-04-25T16:18:01", "subject": "RE: [RFC v2 5/6] eal: add atomic bit operations", "submitter": { "id": 591, "url": "https://patches.dpdk.org/api/people/591/?format=api", "name": "Morten Brørup", "email": "mb@smartsharesystems.com" }, "content": "> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]\n> Sent: Thursday, 25 April 2024 16.36\n> \n> On 2024-04-25 12:25, Morten Brørup wrote:\n> >> +#define rte_bit_atomic_test(addr, nr, memory_order)\n> \t\\\n> >> +\t_Generic((addr),\t\t\t\t\t\t\\\n> >> +\t\t uint32_t *: __rte_bit_atomic_test32,\t\t\t\\\n> >> +\t\t uint64_t *: __rte_bit_atomic_test64)(addr, nr,\n> memory_order)\n> >\n> > I wonder if these should have RTE_ATOMIC qualifier:\n> >\n> > +\t\t RTE_ATOMIC(uint32_t) *: __rte_bit_atomic_test32,\n> \t\t\\\n> > +\t\t RTE_ATOMIC(uint64_t) *: __rte_bit_atomic_test64)(addr, nr,\n> memory_order)\n> >\n> >\n> >> +#define __RTE_GEN_BIT_ATOMIC_TEST(size)\n> \t\\\n> >> +\tstatic inline bool\t\t\t\t\t\t\\\n> >> +\t__rte_bit_atomic_test ## size(const uint ## size ## _t *addr,\n> \t\\\n> >\n> > I wonder if the \"addr\" parameter should have RTE_ATOMIC qualifier:\n> >\n> > +\t__rte_bit_atomic_test ## size(const RTE_ATOMIC(uint ## size ## _t)\n> *addr,\t\\\n> >\n> > instead of casting into a_addr.\n> >\n> \n> Check the cover letter for the rationale for the cast.\n\nThanks, that clarifies it. Then...\nFor the series:\nAcked-by: Morten Brørup <mb@smartsharesystems.com>\n\n> \n> Where I'm at now is that I think C11 _Atomic is rather poor design. The\n> assumption that an object which allows for atomic access always should\n> require all operations upon it to be atomic, regardless of where it is\n> in its lifetime, and which thread is accessing it, does not hold, in the\n> general case.\n\nIt might be slow, but I suppose the C11 standard prioritizes correctness over performance.\n\nIt seems locks are automatically added to _Atomic types larger than what is natively supported by the architecture.\nE.g. MSVC adds locks to _Atomic types larger than 8 byte. [1]\n\n[1]: https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/\n\n> \n> The only reason for _Atomic being as it is, as far as I can see, is to\n> accommodate for ISAs which does not have the appropriate atomic machine\n> instructions, and thus require a lock or some other data associated with\n> the actual user-data-carrying bits. Neither GCC nor DPDK supports any\n> such ISAs, to my knowledge. I suspect neither never will. So the cast\n> will continue to work.\n\nI tend to agree with you on this.\n\nWe should officially decide that DPDK treats RTE_ATOMIC types as a union of _Atomic and non-atomic, i.e. operations on RTE_ATOMIC types can be both atomic and non-atomic.\n\n> \n> >> +\t\t\t\t unsigned int nr, int memory_order) \\\n> >> +\t{\t\t\t\t\t\t\t\t\\\n> >> +\t\tRTE_ASSERT(nr < size);\t\t\t\t\t\\\n> >> +\t\t\t\t\t\t\t\t\t\\\n> >> +\t\tconst RTE_ATOMIC(uint ## size ## _t) *a_addr =\t\t\\\n> >> +\t\t\t(const RTE_ATOMIC(uint ## size ## _t) *)addr;\t\\\n> >> +\t\tuint ## size ## _t mask = (uint ## size ## _t)1 << nr;\t\\\n> >> +\t\treturn rte_atomic_load_explicit(a_addr, memory_order) &\n> mask; \\\n> >> +\t}\n> >\n> >\n> > Similar considerations regarding volatile qualifier for the \"once\"\n> operations.\n> >", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@inbox.dpdk.org", "Delivered-To": "patchwork@inbox.dpdk.org", "Received": [ "from mails.dpdk.org (mails.dpdk.org [217.70.189.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id E944C43F08;\n\tThu, 25 Apr 2024 18:18:07 +0200 (CEST)", "from mails.dpdk.org (localhost [127.0.0.1])\n\tby mails.dpdk.org (Postfix) with ESMTP id B9597438DA;\n\tThu, 25 Apr 2024 18:18:07 +0200 (CEST)", "from dkmailrelay1.smartsharesystems.com\n (smartserver.smartsharesystems.com [77.243.40.215])\n by mails.dpdk.org (Postfix) with ESMTP id 9853B438CF;\n Thu, 25 Apr 2024 18:18:06 +0200 (CEST)", "from smartserver.smartsharesystems.com\n (smartserver.smartsharesys.local [192.168.4.10])\n by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 8F9CE21FDD;\n Thu, 25 Apr 2024 18:18:05 +0200 (CEST)" ], "Content-class": "urn:content-classes:message", "MIME-Version": "1.0", "Content-Type": "text/plain;\n\tcharset=\"UTF-8\"", "Content-Transfer-Encoding": "base64", "Subject": "RE: [RFC v2 5/6] eal: add atomic bit operations", "Date": "Thu, 25 Apr 2024 18:18:01 +0200", "X-MimeOLE": "Produced By Microsoft Exchange V6.5", "Message-ID": "<98CBD80474FA8B44BF855DF32C47DC35E9F3EF@smartserver.smartshare.dk>", "In-Reply-To": "<c36f19cc-6c1c-4d14-8ba4-07a87ef5162a@lysator.liu.se>", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "Thread-Topic": "[RFC v2 5/6] eal: add atomic bit operations", "Thread-Index": "AdqXHfh9lLsThXD/Sf+F6qv23u/DxgADE4jQ", "References": "<20240302135328.531940-2-mattias.ronnblom@ericsson.com>\n <20240425085853.97888-1-mattias.ronnblom@ericsson.com>\n <20240425085853.97888-6-mattias.ronnblom@ericsson.com>\n <98CBD80474FA8B44BF855DF32C47DC35E9F3E8@smartserver.smartshare.dk>\n <c36f19cc-6c1c-4d14-8ba4-07a87ef5162a@lysator.liu.se>", "From": "=?utf-8?q?Morten_Br=C3=B8rup?= <mb@smartsharesystems.com>", "To": "=?utf-8?q?Mattias_R=C3=B6nnblom?= <hofors@lysator.liu.se>, =?utf-8?q?Mat?=\n\t=?utf-8?q?tias_R=C3=B6nnblom?= <mattias.ronnblom@ericsson.com>,\n <dev@dpdk.org>, \"Tyler Retzlaff\" <roretzla@linux.microsoft.com>", "Cc": "\"Heng Wang\" <heng.wang@ericsson.com>,\n \"Stephen Hemminger\" <stephen@networkplumber.org>, <techboard@dpdk.org>", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n <mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org" }, "addressed": null }