From: David Marchand <david.marchand@redhat.com>
To: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
Cc: dev@dpdk.org, Akhil Goyal <gakhil@marvell.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Chenbo Xia <chenbox@nvidia.com>,
Fan Zhang <fanzhang.oss@gmail.com>,
Jay Zhou <jianjay.zhou@huawei.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
jerinj@marvell.com, anoobj@marvell.com,
Rajesh Mudimadugula <rmudimadugul@marvell.com>
Subject: Re: [v1 12/16] common/virtio: common virtio log
Date: Tue, 24 Dec 2024 09:14:48 +0100 [thread overview]
Message-ID: <CAJFAV8z65Dxx2ppbvoMPQiRiwjuHjVvbRCTNKnXbHUgi4RRXhg@mail.gmail.com> (raw)
In-Reply-To: <c6d805e60f33e2b5840520833ce44f2abdbe40f9.1735025264.git.gmuthukrishn@marvell.com>
Hello Gowri,
On Tue, Dec 24, 2024 at 8:39 AM Gowrishankar Muthukrishnan
<gmuthukrishn@marvell.com> wrote:
>
> Common virtio log include file.
That's really a short commitlog..
What are you trying to achieve?
The net/virtio and crypto/virtio drivers had dedicated logtypes so
far, which seems preferable.
I don't see a case when using a single logtype for both net and crypto
would help.
Some comments below.
And please run checkpatch.
>
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> ---
> drivers/{net => common}/virtio/virtio_logs.h | 16 ++--------
> drivers/crypto/virtio/meson.build | 1 +
> .../{virtio_logs.h => virtio_crypto_logs.h} | 30 ++++++++-----------
> drivers/crypto/virtio/virtio_cryptodev.c | 4 +--
> drivers/crypto/virtio/virtqueue.h | 2 +-
> drivers/net/virtio/meson.build | 3 +-
> drivers/net/virtio/virtio.c | 3 +-
> drivers/net/virtio/virtio_ethdev.c | 3 +-
> drivers/net/virtio/virtio_net_logs.h | 30 +++++++++++++++++++
> drivers/net/virtio/virtio_pci.c | 3 +-
> drivers/net/virtio/virtio_pci_ethdev.c | 3 +-
> drivers/net/virtio/virtio_rxtx.c | 3 +-
> drivers/net/virtio/virtio_rxtx_packed.c | 3 +-
> drivers/net/virtio/virtio_rxtx_packed.h | 3 +-
> drivers/net/virtio/virtio_rxtx_packed_avx.h | 3 +-
> drivers/net/virtio/virtio_rxtx_simple.h | 3 +-
> .../net/virtio/virtio_user/vhost_kernel_tap.c | 3 +-
> drivers/net/virtio/virtio_user/vhost_vdpa.c | 3 +-
> drivers/net/virtio/virtio_user_ethdev.c | 3 +-
> drivers/net/virtio/virtqueue.c | 3 +-
> drivers/net/virtio/virtqueue.h | 3 +-
> 21 files changed, 77 insertions(+), 51 deletions(-)
> rename drivers/{net => common}/virtio/virtio_logs.h (61%)
> rename drivers/crypto/virtio/{virtio_logs.h => virtio_crypto_logs.h} (74%)
> create mode 100644 drivers/net/virtio/virtio_net_logs.h
>
> diff --git a/drivers/net/virtio/virtio_logs.h b/drivers/common/virtio/virtio_logs.h
> similarity index 61%
> rename from drivers/net/virtio/virtio_logs.h
> rename to drivers/common/virtio/virtio_logs.h
> index dea1a7ac11..bc115e7a36 100644
> --- a/drivers/net/virtio/virtio_logs.h
> +++ b/drivers/common/virtio/virtio_logs.h
> @@ -5,6 +5,8 @@
> #ifndef _VIRTIO_LOGS_H_
> #define _VIRTIO_LOGS_H_
>
> +#include <inttypes.h>
> +
?
Seems unrelated.
> #include <rte_log.h>
>
> extern int virtio_logtype_init;
> @@ -14,20 +16,6 @@ extern int virtio_logtype_init;
>
> #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
>
> -#ifdef RTE_LIBRTE_VIRTIO_DEBUG_RX
> -#define PMD_RX_LOG(level, ...) \
> - RTE_LOG_LINE_PREFIX(level, VIRTIO_DRIVER, "%s() rx: ", __func__, __VA_ARGS__)
> -#else
> -#define PMD_RX_LOG(...) do { } while(0)
> -#endif
> -
> -#ifdef RTE_LIBRTE_VIRTIO_DEBUG_TX
> -#define PMD_TX_LOG(level, ...) \
> - RTE_LOG_LINE_PREFIX(level, VIRTIO_DRIVER, "%s() tx: ", __func__, __VA_ARGS__)
> -#else
> -#define PMD_TX_LOG(...) do { } while(0)
> -#endif
> -
> extern int virtio_logtype_driver;
> #define RTE_LOGTYPE_VIRTIO_DRIVER virtio_logtype_driver
> #define PMD_DRV_LOG(level, ...) \
> diff --git a/drivers/crypto/virtio/meson.build b/drivers/crypto/virtio/meson.build
> index d2c3b3ad07..6c082a3112 100644
> --- a/drivers/crypto/virtio/meson.build
> +++ b/drivers/crypto/virtio/meson.build
> @@ -8,6 +8,7 @@ if is_windows
> endif
>
> includes += include_directories('../../../lib/vhost')
> +includes += include_directories('../../common/virtio')
There are some special cases when a driver can't rely on meson
dependencies (like order of subdirs evaluation in
drivers/meson.build).
For those special cases, include_directories are used.
But this driver does not seem concerned.
There should be dependencies on vhost and common_virtio instead.
> deps += 'bus_pci'
> sources = files(
> 'virtio_cryptodev.c',
> diff --git a/drivers/crypto/virtio/virtio_logs.h b/drivers/crypto/virtio/virtio_crypto_logs.h
> similarity index 74%
> rename from drivers/crypto/virtio/virtio_logs.h
> rename to drivers/crypto/virtio/virtio_crypto_logs.h
> index 988514919f..56caa162d4 100644
> --- a/drivers/crypto/virtio/virtio_logs.h
> +++ b/drivers/crypto/virtio/virtio_crypto_logs.h
> @@ -2,24 +2,18 @@
> * Copyright(c) 2018 HUAWEI TECHNOLOGIES CO., LTD.
> */
>
> -#ifndef _VIRTIO_LOGS_H_
> -#define _VIRTIO_LOGS_H_
> +#ifndef _VIRTIO_CRYPTO_LOGS_H_
> +#define _VIRTIO_CRYPTO_LOGS_H_
>
> #include <rte_log.h>
>
> -extern int virtio_crypto_logtype_init;
> -#define RTE_LOGTYPE_VIRTIO_CRYPTO_INIT virtio_crypto_logtype_init
> +#include "virtio_logs.h"
>
> -#define PMD_INIT_LOG(level, ...) \
> - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_INIT, "%s(): ", __func__, __VA_ARGS__)
> +extern int virtio_logtype_init;
>
> -#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> -
> -extern int virtio_crypto_logtype_init;
> -#define RTE_LOGTYPE_VIRTIO_CRYPTO_INIT virtio_crypto_logtype_init
> -
> -#define VIRTIO_CRYPTO_INIT_LOG_IMPL(level, ...) \
> - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_INIT, "%s(): ", __func__, __VA_ARGS__)
> +#define VIRTIO_CRYPTO_INIT_LOG_IMPL(level, fmt, args...) \
> + rte_log(RTE_LOG_ ## level, virtio_logtype_init, \
> + "INIT: %s(): " fmt "\n", __func__, ##args)
Don't add back macros directly calling rte_log().
Afaiu, this hunk should be only redirecting
RTE_LOGTYPE_VIRTIO_CRYPTO_INIT to virtio_logtype_init.
>
> #define VIRTIO_CRYPTO_INIT_LOG_INFO(fmt, ...) \
> VIRTIO_CRYPTO_INIT_LOG_IMPL(INFO, fmt, ## __VA_ARGS__)
> @@ -75,11 +69,11 @@ extern int virtio_crypto_logtype_tx;
> #define VIRTIO_CRYPTO_TX_LOG_ERR(fmt, ...) \
> VIRTIO_CRYPTO_TX_LOG_IMPL(ERR, fmt, ## __VA_ARGS__)
>
> -extern int virtio_crypto_logtype_driver;
> -#define RTE_LOGTYPE_VIRTIO_CRYPTO_DRIVER virtio_crypto_logtype_driver
> +extern int virtio_logtype_driver;
>
> -#define VIRTIO_CRYPTO_DRV_LOG_IMPL(level, ...) \
> - RTE_LOG_LINE_PREFIX(level, VIRTIO_CRYPTO_DRIVER, "%s(): ", __func__, __VA_ARGS__)
> +#define VIRTIO_CRYPTO_DRV_LOG_IMPL(level, fmt, args...) \
> + rte_log(RTE_LOG_ ## level, virtio_logtype_driver, \
> + "DRIVER: %s(): " fmt "\n", __func__, ##args)
>
> #define VIRTIO_CRYPTO_DRV_LOG_INFO(fmt, ...) \
> VIRTIO_CRYPTO_DRV_LOG_IMPL(INFO, fmt, ## __VA_ARGS__)
> @@ -90,4 +84,4 @@ extern int virtio_crypto_logtype_driver;
> #define VIRTIO_CRYPTO_DRV_LOG_ERR(fmt, ...) \
> VIRTIO_CRYPTO_DRV_LOG_IMPL(ERR, fmt, ## __VA_ARGS__)
>
> -#endif /* _VIRTIO_LOGS_H_ */
> +#endif /* _VIRTIO_CRYPTO_LOGS_H_ */
> diff --git a/drivers/crypto/virtio/virtio_cryptodev.c b/drivers/crypto/virtio/virtio_cryptodev.c
> index d3db4f898e..b31e7ea0cf 100644
> --- a/drivers/crypto/virtio/virtio_cryptodev.c
> +++ b/drivers/crypto/virtio/virtio_cryptodev.c
> @@ -1749,8 +1749,8 @@ RTE_PMD_REGISTER_PCI(CRYPTODEV_NAME_VIRTIO_PMD, rte_virtio_crypto_driver);
> RTE_PMD_REGISTER_CRYPTO_DRIVER(virtio_crypto_drv,
> rte_virtio_crypto_driver.driver,
> cryptodev_virtio_driver_id);
> -RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_init, init, NOTICE);
> +RTE_LOG_REGISTER_SUFFIX(virtio_logtype_init, init, NOTICE);
> RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_session, session, NOTICE);
> RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_rx, rx, NOTICE);
> RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_tx, tx, NOTICE);
> -RTE_LOG_REGISTER_SUFFIX(virtio_crypto_logtype_driver, driver, NOTICE);
> +RTE_LOG_REGISTER_SUFFIX(virtio_logtype_driver, driver, NOTICE);
> diff --git a/drivers/crypto/virtio/virtqueue.h b/drivers/crypto/virtio/virtqueue.h
> index b31342940e..ccf45800c0 100644
> --- a/drivers/crypto/virtio/virtqueue.h
> +++ b/drivers/crypto/virtio/virtqueue.h
> @@ -15,7 +15,7 @@
> #include "virtio_cvq.h"
> #include "virtio_pci.h"
> #include "virtio_ring.h"
> -#include "virtio_logs.h"
> +#include "virtio_crypto_logs.h"
> #include "virtio_crypto.h"
> #include "virtio_rxtx.h"
>
> diff --git a/drivers/net/virtio/meson.build b/drivers/net/virtio/meson.build
> index 02742da5c2..6331366712 100644
> --- a/drivers/net/virtio/meson.build
> +++ b/drivers/net/virtio/meson.build
> @@ -22,6 +22,7 @@ sources += files(
> 'virtqueue.c',
> )
> deps += ['kvargs', 'bus_pci']
> +includes += include_directories('../../common/virtio')
Idem, this is unneeded, as long as there is a meson dependency (like below).
>
> if arch_subdir == 'x86'
> if cc_has_avx512
> @@ -56,5 +57,5 @@ if is_linux
> 'virtio_user/vhost_user.c',
> 'virtio_user/vhost_vdpa.c',
> 'virtio_user/virtio_user_dev.c')
> - deps += ['bus_vdev']
> + deps += ['bus_vdev', 'common_virtio']
> endif
> diff --git a/drivers/net/virtio/virtio.c b/drivers/net/virtio/virtio.c
> index d9e642f412..21b0490fe7 100644
> --- a/drivers/net/virtio/virtio.c
> +++ b/drivers/net/virtio/virtio.c
> @@ -5,8 +5,9 @@
>
> #include <unistd.h>
>
> +#include "virtio_net_logs.h"
> +
> #include "virtio.h"
> -#include "virtio_logs.h"
>
> uint64_t
> virtio_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 70d4839def..491b75ec19 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -29,9 +29,10 @@
> #include <rte_cycles.h>
> #include <rte_kvargs.h>
>
> +#include "virtio_net_logs.h"
> +
> #include "virtio_ethdev.h"
> #include "virtio.h"
> -#include "virtio_logs.h"
> #include "virtqueue.h"
> #include "virtio_cvq.h"
> #include "virtio_rxtx.h"
> diff --git a/drivers/net/virtio/virtio_net_logs.h b/drivers/net/virtio/virtio_net_logs.h
> new file mode 100644
> index 0000000000..bd5867b1fe
> --- /dev/null
> +++ b/drivers/net/virtio/virtio_net_logs.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation
> + */
> +
> +#ifndef _VIRTIO_NET_LOGS_H_
> +#define _VIRTIO_NET_LOGS_H_
> +
> +#include <inttypes.h>
> +
> +#include <rte_log.h>
> +
> +#include "virtio_logs.h"
> +
> +#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> +
> +#ifdef RTE_LIBRTE_VIRTIO_DEBUG_RX
> +#define PMD_RX_LOG(level, fmt, args...) \
> + RTE_LOG(level, VIRTIO_DRIVER, "%s() rx: " fmt "\n", __func__, ## args)
Rebase damage.
> +#else
> +#define PMD_RX_LOG(level, fmt, args...) do { } while (0)
> +#endif
> +
> +#ifdef RTE_LIBRTE_VIRTIO_DEBUG_TX
> +#define PMD_TX_LOG(level, fmt, args...) \
> + RTE_LOG(level, VIRTIO_DRIVER, "%s() tx: " fmt "\n", __func__, ## args)
> +#else
> +#define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> +#endif
> +
> +#endif /* _VIRTIO_NET_LOGS_H_ */
--
David Marchand
next prev parent reply other threads:[~2024-12-24 8:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-24 7:36 [v1 00/16] crypto/virtio: vDPA and asymmetric support Gowrishankar Muthukrishnan
2024-12-24 7:36 ` [v1 01/16] vhost: include AKCIPHER algorithms in crypto_config Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 02/16] crypto/virtio: remove redundant crypto queue free Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 03/16] crypto/virtio: add asymmetric RSA support Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 04/16] test/crypto: check for RSA capability Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 05/16] test/crypto: return proper codes in create session Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 06/16] test/crypto: add asymmetric tests for virtio PMD Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 07/16] vhost: add asymmetric RSA support Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 08/16] examples/vhost_crypto: add asymmetric support Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 09/16] crypto/virtio: fix dataqueues iteration Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 10/16] crypto/virtio: refactor queue operations Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 11/16] crypto/virtio: add packed ring support Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 12/16] common/virtio: common virtio log Gowrishankar Muthukrishnan
2024-12-24 8:14 ` David Marchand [this message]
2024-12-24 7:37 ` [v1 13/16] common/virtio: move vDPA to common directory Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 14/16] common/virtio: support cryptodev in vdev setup Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 15/16] crypto/virtio: add vhost backend to virtio_user Gowrishankar Muthukrishnan
2024-12-24 7:37 ` [v1 16/16] test/crypto: test virtio_crypto_user PMD Gowrishankar Muthukrishnan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJFAV8z65Dxx2ppbvoMPQiRiwjuHjVvbRCTNKnXbHUgi4RRXhg@mail.gmail.com \
--to=david.marchand@redhat.com \
--cc=anoobj@marvell.com \
--cc=bruce.richardson@intel.com \
--cc=chenbox@nvidia.com \
--cc=dev@dpdk.org \
--cc=fanzhang.oss@gmail.com \
--cc=gakhil@marvell.com \
--cc=gmuthukrishn@marvell.com \
--cc=jerinj@marvell.com \
--cc=jianjay.zhou@huawei.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=maxime.coquelin@redhat.com \
--cc=rmudimadugul@marvell.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).