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