From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 4F67845F2A;
	Tue, 24 Dec 2024 09:15:05 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 1CF9A402AD;
	Tue, 24 Dec 2024 09:15:05 +0100 (CET)
Received: from us-smtp-delivery-124.mimecast.com
 (us-smtp-delivery-124.mimecast.com [170.10.133.124])
 by mails.dpdk.org (Postfix) with ESMTP id CFCE94029D
 for <dev@dpdk.org>; Tue, 24 Dec 2024 09:15:03 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
 s=mimecast20190719; t=1735028103;
 h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
 content-transfer-encoding:content-transfer-encoding:
 in-reply-to:in-reply-to:references:references;
 bh=BNwSQ5/oDAsLqLIGTpaSKq4rdF7xgM/gzPpgUb/wiVY=;
 b=HyeaN9GEdHrimc/EfhX98oxe9dPNQSlMZ4jEEtcZ3AJ0b7tpZ8arlFCZzHoY/6zNJHmlWz
 aOkaK3vsXDC2tvKdbsIUOn/1KQ2IXbXvc9ioHM0RW3Sk3tcgJj6sywU9msbKbC1/UzLzyl
 B4q/UFZ5pEE+Q72PkI7La1jY39VawIk=
Received: from mail-lj1-f198.google.com (mail-lj1-f198.google.com
 [209.85.208.198]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
 us-mta-266-rmarbYijPjONNJHHboI-RA-1; Tue, 24 Dec 2024 03:15:01 -0500
X-MC-Unique: rmarbYijPjONNJHHboI-RA-1
X-Mimecast-MFC-AGG-ID: rmarbYijPjONNJHHboI-RA
Received: by mail-lj1-f198.google.com with SMTP id
 38308e7fff4ca-3012c9f34eeso15364361fa.1
 for <dev@dpdk.org>; Tue, 24 Dec 2024 00:15:01 -0800 (PST)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20230601; t=1735028100; x=1735632900;
 h=content-transfer-encoding:cc:to:subject:message-id:date:from
 :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc
 :subject:date:message-id:reply-to;
 bh=BNwSQ5/oDAsLqLIGTpaSKq4rdF7xgM/gzPpgUb/wiVY=;
 b=EktzSXM/5uT2yKnurwacXYmGmpUhEkbqIxbZGAi35R0k8CxfvKcDYoVloPAiQCWfq+
 Uw1pmnWYKDXNTD82+j5Da1/2b3lp1llKzr0bjbW+d6c8jvuzqu9TOrlyI9p9BGiB8t5i
 6ULQeTGx3OUk+gzGmVzPTT8wUxVq4wvTusjwFSDPvGpbEs3c09Halulq02ZDySlvhnjh
 XvO2dFxoGS1TC1C27WfRHjJGug9x/Xp2FH/hIdohd67SijebV3AMVn6XreY5oIC+SwA5
 pSVo5U1jIWFgJJTjrqPWpracEn6Az0dIqPsM9tafMyihCCLK0ZvGizCvFHUIH5Rhc+Ba
 o5Ag==
X-Gm-Message-State: AOJu0YwxuLs4KjQLCTJAsmL5iM7W7LxAk5PII6lby27wTmd6EyqG28rX
 6IwSAxzedsUcIaOaa46EJlsAOhbX50P/G8KY9W36rhYcotfCwMyCPjxyrTlEtwpH7cwxaE8M8yt
 RT1XCBMyFl/XJ37drPEGPUJu3ault2Z5gNCQqetF8dLniveqrjYQkH3BHC5Iu7K1fooZApgyWnA
 SPEFhlm3M/HHM4wGk=
X-Gm-Gg: ASbGncugfr1/CUt2bkE+xlLSeQ5XB8kK8pIQWI9mf+jUM4Z4f2CbA7YBhDQ5oCC43lO
 COwpBU5HHkNyRxooFbg/Gr9iX/IVKZ65/5wHXu6yy
X-Received: by 2002:a05:6512:334e:b0:542:2972:4e58 with SMTP id
 2adb3069b0e04-54229724f15mr3898547e87.2.1735028099996; 
 Tue, 24 Dec 2024 00:14:59 -0800 (PST)
X-Google-Smtp-Source: AGHT+IFc4kg3jsypmc++SAnIJJ28xmiNWDf+tMDX4iKb5iRfTnN4QSJa2F6+KHNTtXW7Knl32crFr9M4U5uGJRLX0tk=
X-Received: by 2002:a05:6512:334e:b0:542:2972:4e58 with SMTP id
 2adb3069b0e04-54229724f15mr3898536e87.2.1735028099558; Tue, 24 Dec 2024
 00:14:59 -0800 (PST)
MIME-Version: 1.0
References: <cover.1735025264.git.gmuthukrishn@marvell.com>
 <c6d805e60f33e2b5840520833ce44f2abdbe40f9.1735025264.git.gmuthukrishn@marvell.com>
In-Reply-To: <c6d805e60f33e2b5840520833ce44f2abdbe40f9.1735025264.git.gmuthukrishn@marvell.com>
From: David Marchand <david.marchand@redhat.com>
Date: Tue, 24 Dec 2024 09:14:48 +0100
Message-ID: <CAJFAV8z65Dxx2ppbvoMPQiRiwjuHjVvbRCTNKnXbHUgi4RRXhg@mail.gmail.com>
Subject: Re: [v1 12/16] common/virtio: common virtio log
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>
X-Mimecast-Spam-Score: 0
X-Mimecast-MFC-PROC-ID: GbfMSoSdWVptJmOGnvaQYbEJVf6213e_NQIgw_buApE_1735028100
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
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

Hello Gowri,

On Tue, Dec 24, 2024 at 8:39=E2=80=AFAM 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 =3D> common}/virtio/virtio_logs.h  | 16 ++--------
>  drivers/crypto/virtio/meson.build             |  1 +
>  .../{virtio_logs.h =3D> 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 =3D> common}/virtio/virtio_logs.h (61%)
>  rename drivers/crypto/virtio/{virtio_logs.h =3D> 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/vir=
tio_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/me=
son.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 +=3D include_directories('../../../lib/vhost')
> +includes +=3D 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 +=3D 'bus_pci'
>  sources =3D 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/vi=
rtio/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/vi=
rtqueue.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.bu=
ild
> index 02742da5c2..6331366712 100644
> --- a/drivers/net/virtio/meson.build
> +++ b/drivers/net/virtio/meson.build
> @@ -22,6 +22,7 @@ sources +=3D files(
>          'virtqueue.c',
>  )
>  deps +=3D ['kvargs', 'bus_pci']
> +includes +=3D include_directories('../../common/virtio')

Idem, this is unneeded, as long as there is a meson dependency (like below)=
.


>
>  if arch_subdir =3D=3D '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 +=3D ['bus_vdev']
> +    deps +=3D ['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/virt=
io_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/vi=
rtio_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_ */


--=20
David Marchand