DPDK patches and discussions
 help / color / mirror / Atom feed
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


  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).