From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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: In-Reply-To: From: David Marchand Date: Tue, 24 Dec 2024 09:14:48 +0100 Message-ID: Subject: Re: [v1 12/16] common/virtio: common virtio log To: Gowrishankar Muthukrishnan Cc: dev@dpdk.org, Akhil Goyal , Maxime Coquelin , Chenbo Xia , Fan Zhang , Jay Zhou , Bruce Richardson , Konstantin Ananyev , jerinj@marvell.com, anoobj@marvell.com, Rajesh Mudimadugula 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hello Gowri, On Tue, Dec 24, 2024 at 8:39=E2=80=AFAM Gowrishankar Muthukrishnan 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 > --- > 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 > + ? Seems unrelated. > #include > > 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 > > -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 > > +#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 > #include > > +#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 > + > +#include > + > +#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