From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D6F5EA0C58; Thu, 12 Aug 2021 23:58:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7398C4014D; Thu, 12 Aug 2021 23:58:42 +0200 (CEST) Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by mails.dpdk.org (Postfix) with ESMTP id 4129140042 for ; Thu, 12 Aug 2021 23:58:41 +0200 (CEST) Received: by mail-lj1-f180.google.com with SMTP id q11so3191946ljp.4 for ; Thu, 12 Aug 2021 14:58:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=cfJm5vfF3/DMTEZk1wvrxDAbQDgC4bxi2spKkX/mFHY=; b=h5H928PNJzYO4MA5P9+UgQw0WI+esHvRDenioSjyGh+j5IM+/ajBKOJtMW/PxjLl4W x7Lk5qkdiHK/HEuTi4J+EVABFKwC35Vpz99GgZzmOBmiXj9CHYv9D6C1l5NW/UWBrUWD 3JvRBbFm0GE+Q5wnQ8vgodytrsUOM+wmtwnHs5ua52OEYi5YSGZrRxYvoLPQJvVuBdFe Xp+6rc7YJUb73kKNQ8nhPFfH9SMYwdtWcFwXzz8tISY1wIQkPOyRpaGz4ARS5NEeCoHs umXDHSdDRfMOFV5bO50YhEnG1qImZ4xiQFhkgwEL04VPoEhrc2ON4cxqu65Wr0nAXvT+ 2ojg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=cfJm5vfF3/DMTEZk1wvrxDAbQDgC4bxi2spKkX/mFHY=; b=jVLb2T3SD4IlMh+LajB1dtxNuuB1QO/l8AFn1LoZsa9EB5MIBqwCEw3TKnGMHmq7z0 T/9iOH2cXZT5Y/mWj3yRYTnDAO9SeBAP6g1IadUwIylAJt8ays/uJ2ElEM4y1Jq4aKR4 b4fLRdsOvE4i8tZLcZUzrH2r/4M5fcxFtguAhPus82RSlSbwcrp88Myjpa2DjeHjvmAp 2bsiBk8yKB36PrtqhEmZy9EIovvo/3uwS6K47I4SZa8b+3aPvln5arNTjWgm50Xrp7/c yFWB63LMYISncN0aByCsQv8N8vuYQXVhV0mAmMrfQmdYFDSDnpG6mzwigBMndkV7GryZ k4/g== X-Gm-Message-State: AOAM533CRon4mCWsFLtmnd58B47w39qOIxFJJK5DfNp7RHoWEUYUXb2k YMaN8lOnZCuKOF3dcJEVyeI= X-Google-Smtp-Source: ABdhPJyzdkVUO/Vxnc86Kh9jmYwitp9sppSWGune//WRUblBsZxNqPj58r6hauGFj4qVPXSz3HRQig== X-Received: by 2002:a2e:9d0a:: with SMTP id t10mr275293lji.282.1628805520773; Thu, 12 Aug 2021 14:58:40 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id v6sm381912lfq.32.2021.08.12.14.58.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Aug 2021 14:58:39 -0700 (PDT) Date: Fri, 13 Aug 2021 00:58:38 +0300 From: Dmitry Kozlyuk To: William Tu Cc: dev@dpdk.org, nick.connolly@mayadata.io Message-ID: <20210813005838.2655c74f@sovereign> In-Reply-To: <20210812200528.60743-1-u9012063@gmail.com> References: <20210811204627.213-1-u9012063@gmail.com> <20210812200528.60743-1-u9012063@gmail.com> X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCHv3] include: fix sys/queue.h. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2021-08-12 20:05 (UTC+0000), William Tu: > Currently there are a couple of public header files include Suggested subject: "eal: remove sys/queue.h from public headers". 1. The state before the patch should be described in the past tense. 2. Really ten times more than "a couple", suggesting "some" (nit). 2. "files _that_ include"? > 'sys/queue.h', which is a POSIX functionality. It's not POSIX, it's found on many Unix systems. > When compiling DPDK with OVS on Windows, we encountered issues such as, found the missing > header. This sentence is a little hard to parse. Instead, suggesting: This file is missing on Windows. During the build, DPDK uses a bundled copy, but it cannot be installed because macros it exports may conflict with the ones from application code or environment. > In file included from ../lib/dpdk.c:27: > C:\temp\dpdk\include\rte_log.h:24:10: fatal error: 'sys/queue.h' file > not found An explanation is missing why embedded in DPDK shouldn't be installed (see above, maybe you can come up with something better). > > The patch fixes it by removing the #include from > DPDK public headers, so programs including DPDK headers don't depend > on POSIX sys/queue.h. For Linux/FreeBSD, DPDK public headers only need a > handful of macros for list/tailq heads and links. Those macros should be > provided by DPDK, with RTE_ prefix. It is worth noting that RTE_ macros must be compatible with at the level of API (to use with macros in C files) and ABI (to avoid breaking it). Nit: "Should" is not the right word for things done in the patch. Same below. > For Linux and FreeBSD it will just be: > #include > #define RTE_TAILQ_ENTRY(type) TAILQ_ENTRY(type) > /* ... */ > For Windows, we copy these definitions from to rte_os.h. No need to describe what's inside the patch, diff already does it :) > With this patch, all the public headers should not have > "#include " or "TAILQ_xxx" macros. > > Suggested-by: Nick Connolly > Suggested-by: Dmitry Kozliuk > Signed-off-by: William Tu > --- > v2->v3: > * follow the suggestion by Dmitry > * run checkpatches, there are some errors but I think either > the original file has over 80-char line due to comments, > or some false positive about macro. > v1->v2: > - follow the suggestion by Nick and Dmitry > - http://mails.dpdk.org/archives/dev/2021-August/216304.html > > Signed-off-by: William Tu > --- [...] > diff --git a/lib/eal/freebsd/include/rte_os.h b/lib/eal/freebsd/include/rte_os.h > index 627f0483ab..dc889e5826 100644 > --- a/lib/eal/freebsd/include/rte_os.h > +++ b/lib/eal/freebsd/include/rte_os.h > @@ -11,6 +11,39 @@ > */ > > #include > +#include > + > +/* These macros are compatible with system's sys/queue.h. */ > +#define RTE_TAILQ_INIT(head) TAILQ_INIT(head) > +#define RTE_TAILQ_HEAD(name, type) TAILQ_HEAD(name, type) > +#define RTE_TAILQ_LAST(head, headname) TAILQ_LAST(head, headname) > +#define RTE_TAILQ_ENTRY(type) TAILQ_ENTRY(type) > +#define RTE_TAILQ_FIRST(head) TAILQ_FIRST(head) > +#define RTE_TAILQ_EMPTY(head) TAILQ_EMPTY(head) > +#define RTE_TAILQ_NEXT(elem, field) TAILQ_NEXT(elem, field) > +#define RTE_TAILQ_HEAD_INITIALIZER(head) TAILQ_HEAD_INITIALIZER(head) > +#define RTE_TAILQ_FOREACH(var, head, field) TAILQ_FOREACH(var, head, field) > +#define RTE_TAILQ_INSERT_TAIL(head, elm, field) \ > + TAILQ_INSERT_TAIL(head, elm, field) > +#define RTE_TAILQ_REMOVE(head, elm, field) TAILQ_REMOVE(head, elm, field) > +#define RTE_TAILQ_INSERT_BEFORE(listelm, elm, field) \ > + TAILQ_INSERT_BEFORE(listelm, elm, field) > +#define RTE_TAILQ_INSERT_AFTER(head, listelm, elm, field) \ > + TAILQ_INSERT_AFTER(head, listelm, elm, field) > +#define RTE_TAILQ_INSERT_HEAD(head, elm, field) \ > + TAILQ_INSERT_HEAD(head, elm, field) > + > +#define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type) > +#define RTE_STAILQ_HEAD_INITIALIZER(head) STAILQ_HEAD_INITIALIZER(head) > +#define RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type) Most of these macros are not used in public headers and are not needed. The idea is that TAILQ_* macros from sys/queue.h can be used in C files with variables declared with RTE_TAILQ_HEAD/ENTRY in public headers. Needed macros: RTE_TAILQ_HEAD RTE_TAILQ_ENTRY RTE_TAILQ_FOREACH RTE_TAILQ_FIRST (for RTE_TAILQ_FOREACH_SAFE only) RTE_TAILQ_NEXT (ditto) RTE_STAILQ_HEAD RTE_STAILQ_ENTRY > + > +/* This is not defined in sys/queue.h */ > +#ifndef TAILQ_FOREACH_SAFE > +#define TAILQ_FOREACH_SAFE(var, head, field, tvar) \ > + for ((var) = RTE_TAILQ_FIRST((head)); \ > + (var) && ((tvar) = RTE_TAILQ_NEXT((var), field), 1); \ > + (var) = (tvar)) > +#endif Please simply change the three usages of TAILQ_FOREACH_SAFE to RTE_TAILQ_FOREACH_SAFE and remove this one. It cannot be placed in rte_os.h, because rte_os.h is public and it must not export non-RTE symbols. All comments to this file obviously apply to Linux version as well. > > typedef cpuset_t rte_cpuset_t; > #define RTE_HAS_CPUSET [...] > diff --git a/lib/eal/windows/include/rte_os.h b/lib/eal/windows/include/rte_os.h > index 66c711d458..d0935c5003 100644 > --- a/lib/eal/windows/include/rte_os.h > +++ b/lib/eal/windows/include/rte_os.h > @@ -18,6 +18,144 @@ > extern "C" { > #endif > > +#ifdef QUEUE_MACRO_DEBUG_TRACE IMO we all these debugging macros should be removed from this header, including their use in user-facing macros. They are implementation detail for developers. > +/* Store the last 2 places the queue element or head was altered */ > +struct qm_trace { > + unsigned long lastline; > + unsigned long prevline; > + const char *lastfile; > + const char *prevfile; > +}; > + > +/** > + * These macros are compatible with the sys/queue.h provided > + * at DPDK source code. > + */ [...] > + > +#define QMD_TAILQ_CHECK_HEAD(head, field) > +#define QMD_TAILQ_CHECK_TAIL(head, headname) > +#define QMD_TAILQ_CHECK_NEXT(elm, field) > +#define QMD_TAILQ_CHECK_PREV(elm, field) Redundant empty lines below. > + > + > +#define RTE_TAILQ_EMPTY(head) ((head)->tqh_first == NULL) > + > +#define RTE_TAILQ_FIRST(head) ((head)->tqh_first) > + > +#define RTE_TAILQ_INIT(head) do { \ I suggest removing all spaces but one before the backslash so that you don't need to manually align. At least please keep the lines within 80 characters. > + RTE_TAILQ_FIRST((head)) = NULL; \ > + (head)->tqh_last = &RTE_TAILQ_FIRST((head)); \ > + QMD_TRACE_HEAD(head); \ > +} while (0) [...]