Message ID | 20230120182154.481039-1-bruce.richardson@intel.com (mailing list archive) |
---|---|
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]) by inbox.dpdk.org (Postfix) with ESMTP id E8FBC42439; Fri, 20 Jan 2023 19:22:16 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 862C242D14; Fri, 20 Jan 2023 19:22:16 +0100 (CET) Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 2EC8A4067E for <dev@dpdk.org>; Fri, 20 Jan 2023 19:22:14 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1674238935; x=1705774935; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=X1JUB5/HJPshrjPh94H4qKiYVd18+5dn590aVNt771I=; b=c/i/hcyM5Zbc2MeCSaGDXlR1MW4QL2ee8HbV24UElln5Oj1eIVuj5c8G Rb5/hEi+s0GMNXARYoKA/oeIomSZN5dHKCxTLjlZyz2p8urqSzF04GgTY VT07qy8xNeUA6U0lRn3Ue3hdn7CcefX7eK6YLTe3dHYzoDb7GAl6T1viN Jy2e13cyTv4fre70PwLFnllRinXLjGEjM7wEyrTpLzIlMT8z5aOSx/DGl cXvTzjCgzPQ2/fpqVLfaKEp3g1Qj6RBYp1TMVThugJyyCnTOAzpTkDI75 YPyfD1A/aEkZad0cW0v0VhZKP3rEqJGlvOf04ghCK0pSBBP/Q8VGClP/I w==; X-IronPort-AV: E=McAfee;i="6500,9779,10596"; a="388012436" X-IronPort-AV: E=Sophos;i="5.97,233,1669104000"; d="scan'208";a="388012436" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Jan 2023 10:22:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10596"; a="691143400" X-IronPort-AV: E=Sophos;i="5.97,233,1669104000"; d="scan'208";a="691143400" Received: from silpixa00401385.ir.intel.com ([10.237.214.55]) by orsmga008.jf.intel.com with ESMTP; 20 Jan 2023 10:22:03 -0800 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: david.marchand@redhat.com, Bruce Richardson <bruce.richardson@intel.com> Subject: [PATCH v4 0/3] Split logging functionality out of EAL Date: Fri, 20 Jan 2023 18:21:51 +0000 Message-Id: <20230120182154.481039-1-bruce.richardson@intel.com> X-Mailer: git-send-email 2.37.2 In-Reply-To: <20220829151901.376754-1-bruce.richardson@intel.com> References: <20220829151901.376754-1-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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>, <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>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
Series |
Split logging functionality out of EAL
|
|
Message
Bruce Richardson
Jan. 20, 2023, 6:21 p.m. UTC
There is a general desire to reduce the size and scope of EAL. To this end, this patchset makes a (very) small step in that direction by taking the logging functionality out of EAL and putting it into its own library that can be built and maintained separately. As with the first RFC for this, the main obstacle is the "fnmatch" function which is needed by both EAL and the new log function when building on windows. While the function cannot stay in EAL - or we would have a circular dependency, moving it to a new library or just putting it in the log library have the disadvantages that it then "leaks" into the public namespace without an rte_prefix, which could cause issues. Since only a single function is involved, subsequent versions take a different approach to v1, and just moves the offending function to be a static function in a header file. This allows use by multiple libs without conflicting names or making it public. The other complication, as explained in v1 RFC was that of multiple implementations for different OS's. This is solved here in the same way as v1, by including the OS in the name and having meson pick the correct file for each build. Since only one file is involved, there seemed little need for replicating EAL's separate subdirectories per-OS. v4: * Fixed windows build error, due to missing strdup (_strdup on windows) * Added doc updates to programmers guide. v3: * Fixed missing log file for BSD * Removed "eal" from the filenames of files in the log directory * added prefixes to elements in the fnmatch header to avoid conflicts * fixed space indentation in new lines in telemetry.c (checkpatch) * removed "extern int logtype" definition in telemetry.c (checkpatch) * added log directory to list for doxygen scanning Bruce Richardson (3): eal/windows: move fnmatch function to header file log: separate logging functions out of EAL telemetry: use standard logging doc/api/doxy-api.conf.in | 1 + .../prog_guide/env_abstraction_layer.rst | 4 +- doc/guides/prog_guide/index.rst | 1 + doc/guides/prog_guide/log_lib.rst | 115 ++++++++++++ lib/eal/common/eal_common_options.c | 2 +- lib/eal/common/eal_private.h | 7 - lib/eal/common/meson.build | 1 - lib/eal/freebsd/eal.c | 6 +- lib/eal/include/meson.build | 1 - lib/eal/linux/eal.c | 8 +- lib/eal/linux/meson.build | 1 - lib/eal/meson.build | 2 +- lib/eal/version.map | 17 -- lib/eal/windows/eal.c | 2 +- lib/eal/windows/fnmatch.c | 172 ----------------- lib/eal/windows/include/fnmatch.h | 175 ++++++++++++++++-- lib/eal/windows/meson.build | 2 - lib/kvargs/meson.build | 3 +- .../common/eal_common_log.c => log/log.c} | 7 +- lib/log/log_freebsd.c | 12 ++ .../common/eal_log.h => log/log_internal.h} | 18 +- lib/{eal/linux/eal_log.c => log/log_linux.c} | 2 +- .../windows/eal_log.c => log/log_windows.c} | 2 +- lib/log/meson.build | 9 + lib/{eal/include => log}/rte_log.h | 0 lib/log/version.map | 34 ++++ lib/meson.build | 1 + lib/telemetry/meson.build | 3 +- lib/telemetry/telemetry.c | 11 +- lib/telemetry/telemetry_internal.h | 3 +- 30 files changed, 370 insertions(+), 252 deletions(-) create mode 100644 doc/guides/prog_guide/log_lib.rst delete mode 100644 lib/eal/windows/fnmatch.c rename lib/{eal/common/eal_common_log.c => log/log.c} (99%) create mode 100644 lib/log/log_freebsd.c rename lib/{eal/common/eal_log.h => log/log_internal.h} (69%) rename lib/{eal/linux/eal_log.c => log/log_linux.c} (97%) rename lib/{eal/windows/eal_log.c => log/log_windows.c} (93%) create mode 100644 lib/log/meson.build rename lib/{eal/include => log}/rte_log.h (100%) create mode 100644 lib/log/version.map -- 2.37.2
Comments
Hi Bruce, On Fri, Jan 20, 2023 at 7:22 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > There is a general desire to reduce the size and scope of EAL. To this > end, this patchset makes a (very) small step in that direction by taking > the logging functionality out of EAL and putting it into its own library > that can be built and maintained separately. > > As with the first RFC for this, the main obstacle is the "fnmatch" > function which is needed by both EAL and the new log function when > building on windows. While the function cannot stay in EAL - or we would > have a circular dependency, moving it to a new library or just putting > it in the log library have the disadvantages that it then "leaks" into > the public namespace without an rte_prefix, which could cause issues. > Since only a single function is involved, subsequent versions take a > different approach to v1, and just moves the offending function to be a > static function in a header file. This allows use by multiple libs > without conflicting names or making it public. > > The other complication, as explained in v1 RFC was that of multiple > implementations for different OS's. This is solved here in the same > way as v1, by including the OS in the name and having meson pick the > correct file for each build. Since only one file is involved, there > seemed little need for replicating EAL's separate subdirectories > per-OS. There is another complication. The ABI check is not handling properly the case where symbols are moved to the new log library (even though the dependency to librte_log is explicit in librte_eal elf). For now, I don't have a good way to handle this. A workaround to pass the check is to suppress those symbols wrt the eal dump: [suppress_function] symbol_name_regexp = rte_log [suppress_function] symbol_name = rte_openlog_stream [suppress_function] symbol_name = rte_vlog But this is not a good solution because we would be losing checks on them for the rest of the v23 ABI life.
On Sun, Jan 22, 2023 at 03:56:12PM +0100, David Marchand wrote: > Hi Bruce, > > On Fri, Jan 20, 2023 at 7:22 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > There is a general desire to reduce the size and scope of EAL. To this > > end, this patchset makes a (very) small step in that direction by taking > > the logging functionality out of EAL and putting it into its own library > > that can be built and maintained separately. > > > > As with the first RFC for this, the main obstacle is the "fnmatch" > > function which is needed by both EAL and the new log function when > > building on windows. While the function cannot stay in EAL - or we would > > have a circular dependency, moving it to a new library or just putting > > it in the log library have the disadvantages that it then "leaks" into > > the public namespace without an rte_prefix, which could cause issues. > > Since only a single function is involved, subsequent versions take a > > different approach to v1, and just moves the offending function to be a > > static function in a header file. This allows use by multiple libs > > without conflicting names or making it public. > > > > The other complication, as explained in v1 RFC was that of multiple > > implementations for different OS's. This is solved here in the same > > way as v1, by including the OS in the name and having meson pick the > > correct file for each build. Since only one file is involved, there > > seemed little need for replicating EAL's separate subdirectories > > per-OS. > > There is another complication. > > The ABI check is not handling properly the case where symbols are > moved to the new log library (even though the dependency to librte_log > is explicit in librte_eal elf). > For now, I don't have a good way to handle this. > > A workaround to pass the check is to suppress those symbols wrt the eal dump: > [suppress_function] > symbol_name_regexp = rte_log > [suppress_function] > symbol_name = rte_openlog_stream > [suppress_function] > symbol_name = rte_vlog > > But this is not a good solution because we would be losing checks on > them for the rest of the v23 ABI life. > Right, I got error messages from the CI job for this too, but I also have no idea how to work around this. Perhaps we only get to move content between libraries when we do an ABI bump? Seems a bit restrictive, though. /Bruce
On Mon, Jan 23, 2023 at 3:24 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > On Sun, Jan 22, 2023 at 03:56:12PM +0100, David Marchand wrote: > > Hi Bruce, > > > > On Fri, Jan 20, 2023 at 7:22 PM Bruce Richardson > > <bruce.richardson@intel.com> wrote: > > > > > > There is a general desire to reduce the size and scope of EAL. To this > > > end, this patchset makes a (very) small step in that direction by taking > > > the logging functionality out of EAL and putting it into its own library > > > that can be built and maintained separately. > > > > > > As with the first RFC for this, the main obstacle is the "fnmatch" > > > function which is needed by both EAL and the new log function when > > > building on windows. While the function cannot stay in EAL - or we would > > > have a circular dependency, moving it to a new library or just putting > > > it in the log library have the disadvantages that it then "leaks" into > > > the public namespace without an rte_prefix, which could cause issues. > > > Since only a single function is involved, subsequent versions take a > > > different approach to v1, and just moves the offending function to be a > > > static function in a header file. This allows use by multiple libs > > > without conflicting names or making it public. > > > > > > The other complication, as explained in v1 RFC was that of multiple > > > implementations for different OS's. This is solved here in the same > > > way as v1, by including the OS in the name and having meson pick the > > > correct file for each build. Since only one file is involved, there > > > seemed little need for replicating EAL's separate subdirectories > > > per-OS. > > > > There is another complication. > > > > The ABI check is not handling properly the case where symbols are > > moved to the new log library (even though the dependency to librte_log > > is explicit in librte_eal elf). > > For now, I don't have a good way to handle this. > > > > A workaround to pass the check is to suppress those symbols wrt the eal dump: > > [suppress_function] > > symbol_name_regexp = rte_log > > [suppress_function] > > symbol_name = rte_openlog_stream > > [suppress_function] > > symbol_name = rte_vlog > > > > But this is not a good solution because we would be losing checks on > > them for the rest of the v23 ABI life. > > > Right, I got error messages from the CI job for this too, but I also have > no idea how to work around this. Perhaps we only get to move content > between libraries when we do an ABI bump? Seems a bit restrictive, though. Moving symbols as you did does not seem an ABI breakage. An application that links to eal would see the dt_needed entry for the new log library, load it accordingly and gets the right symbols.
On Mon, Jan 23, 2023 at 03:31:58PM +0100, David Marchand wrote: > On Mon, Jan 23, 2023 at 3:24 PM Bruce Richardson > <bruce.richardson@intel.com> wrote: > > > > On Sun, Jan 22, 2023 at 03:56:12PM +0100, David Marchand wrote: > > > Hi Bruce, > > > > > > On Fri, Jan 20, 2023 at 7:22 PM Bruce Richardson > > > <bruce.richardson@intel.com> wrote: > > > > > > > > There is a general desire to reduce the size and scope of EAL. To this > > > > end, this patchset makes a (very) small step in that direction by taking > > > > the logging functionality out of EAL and putting it into its own library > > > > that can be built and maintained separately. > > > > > > > > As with the first RFC for this, the main obstacle is the "fnmatch" > > > > function which is needed by both EAL and the new log function when > > > > building on windows. While the function cannot stay in EAL - or we would > > > > have a circular dependency, moving it to a new library or just putting > > > > it in the log library have the disadvantages that it then "leaks" into > > > > the public namespace without an rte_prefix, which could cause issues. > > > > Since only a single function is involved, subsequent versions take a > > > > different approach to v1, and just moves the offending function to be a > > > > static function in a header file. This allows use by multiple libs > > > > without conflicting names or making it public. > > > > > > > > The other complication, as explained in v1 RFC was that of multiple > > > > implementations for different OS's. This is solved here in the same > > > > way as v1, by including the OS in the name and having meson pick the > > > > correct file for each build. Since only one file is involved, there > > > > seemed little need for replicating EAL's separate subdirectories > > > > per-OS. > > > > > > There is another complication. > > > > > > The ABI check is not handling properly the case where symbols are > > > moved to the new log library (even though the dependency to librte_log > > > is explicit in librte_eal elf). > > > For now, I don't have a good way to handle this. > > > > > > A workaround to pass the check is to suppress those symbols wrt the eal dump: > > > [suppress_function] > > > symbol_name_regexp = rte_log > > > [suppress_function] > > > symbol_name = rte_openlog_stream > > > [suppress_function] > > > symbol_name = rte_vlog > > > > > > But this is not a good solution because we would be losing checks on > > > them for the rest of the v23 ABI life. > > > > > Right, I got error messages from the CI job for this too, but I also have > > no idea how to work around this. Perhaps we only get to move content > > between libraries when we do an ABI bump? Seems a bit restrictive, though. > > Moving symbols as you did does not seem an ABI breakage. > An application that links to eal would see the dt_needed entry for the > new log library, load it accordingly and gets the right symbols. > Yes, I agree. However, I also agree with you that it is risky to lose symbol checking for the moved symbols if we need to remove them from analysis. That said, maybe others have some ideas as to how to work around this, or perhaps we just take the risk of disabling checking. /Bruce
On Mon, Jan 23, 2023 at 3:37 PM Bruce Richardson <bruce.richardson@intel.com> wrote: > > On Mon, Jan 23, 2023 at 03:31:58PM +0100, David Marchand wrote: > > On Mon, Jan 23, 2023 at 3:24 PM Bruce Richardson > > <bruce.richardson@intel.com> wrote: > > > > > > On Sun, Jan 22, 2023 at 03:56:12PM +0100, David Marchand wrote: > > > > Hi Bruce, > > > > > > > > On Fri, Jan 20, 2023 at 7:22 PM Bruce Richardson > > > > <bruce.richardson@intel.com> wrote: > > > > > > > > > > There is a general desire to reduce the size and scope of EAL. To this > > > > > end, this patchset makes a (very) small step in that direction by taking > > > > > the logging functionality out of EAL and putting it into its own library > > > > > that can be built and maintained separately. > > > > > > > > > > As with the first RFC for this, the main obstacle is the "fnmatch" > > > > > function which is needed by both EAL and the new log function when > > > > > building on windows. While the function cannot stay in EAL - or we would > > > > > have a circular dependency, moving it to a new library or just putting > > > > > it in the log library have the disadvantages that it then "leaks" into > > > > > the public namespace without an rte_prefix, which could cause issues. > > > > > Since only a single function is involved, subsequent versions take a > > > > > different approach to v1, and just moves the offending function to be a > > > > > static function in a header file. This allows use by multiple libs > > > > > without conflicting names or making it public. > > > > > > > > > > The other complication, as explained in v1 RFC was that of multiple > > > > > implementations for different OS's. This is solved here in the same > > > > > way as v1, by including the OS in the name and having meson pick the > > > > > correct file for each build. Since only one file is involved, there > > > > > seemed little need for replicating EAL's separate subdirectories > > > > > per-OS. > > > > > > > > There is another complication. > > > > > > > > The ABI check is not handling properly the case where symbols are > > > > moved to the new log library (even though the dependency to librte_log > > > > is explicit in librte_eal elf). > > > > For now, I don't have a good way to handle this. > > > > > > > > A workaround to pass the check is to suppress those symbols wrt the eal dump: > > > > [suppress_function] > > > > symbol_name_regexp = rte_log > > > > [suppress_function] > > > > symbol_name = rte_openlog_stream > > > > [suppress_function] > > > > symbol_name = rte_vlog > > > > > > > > But this is not a good solution because we would be losing checks on > > > > them for the rest of the v23 ABI life. > > > > > > > Right, I got error messages from the CI job for this too, but I also have > > > no idea how to work around this. Perhaps we only get to move content > > > between libraries when we do an ABI bump? Seems a bit restrictive, though. > > > > Moving symbols as you did does not seem an ABI breakage. > > An application that links to eal would see the dt_needed entry for the > > new log library, load it accordingly and gets the right symbols. > > > Yes, I agree. However, I also agree with you that it is risky to lose > symbol checking for the moved symbols if we need to remove them from > analysis. That said, maybe others have some ideas as to how to work around > this, or perhaps we just take the risk of disabling checking. I opened a bz for libabigail. https://sourceware.org/bugzilla/show_bug.cgi?id=30034 If it is handled fast enough, we may have a solution by the time 23.03 is released and we will remove this workaround for 23.07 development (no pressure Dodji :-p).