[v1,03/17] ethdev: replace library debug flag with global one
Checks
Commit Message
Use global debug flag RTE_DEBUG instead of RTE_LIBRTE_ETHDEV_DEBUG.
The old define is completely removed from source code and config.
The changes were applied also to all drivers using this flag.
Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
config/common_base | 1 -
drivers/net/atlantic/atl_rxtx.c | 2 +-
drivers/net/e1000/em_rxtx.c | 2 +-
drivers/net/e1000/igb_rxtx.c | 2 +-
drivers/net/ena/ena_ethdev.c | 2 +-
drivers/net/enic/enic_rxtx.c | 2 +-
drivers/net/fm10k/fm10k_rxtx.c | 2 +-
drivers/net/hinic/hinic_pmd_tx.c | 2 +-
drivers/net/hns3/hns3_rxtx.c | 2 +-
drivers/net/i40e/i40e_rxtx.c | 2 +-
drivers/net/iavf/iavf_rxtx.c | 2 +-
drivers/net/ice/ice_rxtx.c | 2 +-
drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
drivers/net/qede/qede_rxtx.c | 4 ++--
drivers/net/softnic/rte_eth_softnic.c | 2 +-
drivers/net/softnic/rte_eth_softnic_thread.c | 2 +-
drivers/net/virtio/virtio_rxtx.c | 2 +-
drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
lib/librte_net/rte_net.h | 4 ++--
20 files changed, 28 insertions(+), 29 deletions(-)
Comments
> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Friday, April 17, 2020 10:57 PM
> To: Thomas Monjalon <thomas@monjalon.net>; Igor Russkikh
> <igor.russkikh@aquantia.com>; Pavel Belous <pavel.belous@aquantia.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Marcin Wojtas
> <mw@semihalf.com>; Michal Krawczyk <mk@semihalf.com>; Guy Tzalik
> <gtzalik@amazon.com>; Evgeny Schemeilin <evgenys@amazon.com>; Igor
> Chauskin <igorch@amazon.com>; John Daley <johndale@cisco.com>; Hyong
> Youb Kim <hyonkim@cisco.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Wang, Xiao W <xiao.w.wang@intel.com>; Ziyang Xuan
> <xuanziyang2@huawei.com>; Xiaoyun Wang
> <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
> <zhouguoyang@huawei.com>; Wei Hu (Xavier)
> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
> Yisen Zhuang <yisen.zhuang@huawei.com>; Xing, Beilei
> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Rasesh Mody <rmody@marvell.com>;
> Shahed Shaikh <shshaikh@marvell.com>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yong
> Wang <yongwang@vmware.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> <olivier.matz@6wind.com>
> Cc: dev@dpdk.org
> Subject: [PATCH v1 03/17] ethdev: replace library debug flag with global one
>
> Use global debug flag RTE_DEBUG instead of RTE_LIBRTE_ETHDEV_DEBUG.
> The old define is completely removed from source code and config.
> The changes were applied also to all drivers using this flag.
>
> Signed-off-by: Lukasz Wojciechowski
> <l.wojciechow@partner.samsung.com>
> ---
> config/common_base | 1 -
> drivers/net/atlantic/atl_rxtx.c | 2 +-
> drivers/net/e1000/em_rxtx.c | 2 +-
> drivers/net/e1000/igb_rxtx.c | 2 +-
> drivers/net/ena/ena_ethdev.c | 2 +-
> drivers/net/enic/enic_rxtx.c | 2 +-
> drivers/net/fm10k/fm10k_rxtx.c | 2 +-
> drivers/net/hinic/hinic_pmd_tx.c | 2 +-
> drivers/net/hns3/hns3_rxtx.c | 2 +-
> drivers/net/i40e/i40e_rxtx.c | 2 +-
> drivers/net/iavf/iavf_rxtx.c | 2 +-
> drivers/net/ice/ice_rxtx.c | 2 +-
> drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
> drivers/net/qede/qede_rxtx.c | 4 ++--
> drivers/net/softnic/rte_eth_softnic.c | 2 +-
> drivers/net/softnic/rte_eth_softnic_thread.c | 2 +-
> drivers/net/virtio/virtio_rxtx.c | 2 +-
> drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
> lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
> lib/librte_net/rte_net.h | 4 ++--
> 20 files changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/config/common_base b/config/common_base
> index c5be57f11..16a8f09b6 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -149,7 +149,6 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> # Compile generic ethernet library
> #
> CONFIG_RTE_LIBRTE_ETHER=y
> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> CONFIG_RTE_MAX_ETHPORTS=32
> CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> CONFIG_RTE_LIBRTE_IEEE1588=n
> diff --git a/drivers/net/atlantic/atl_rxtx.c b/drivers/net/atlantic/atl_rxtx.c
> index 449ffd454..eae54df22 100644
> --- a/drivers/net/atlantic/atl_rxtx.c
> +++ b/drivers/net/atlantic/atl_rxtx.c
> @@ -821,7 +821,7 @@ atl_prep_pkts(__rte_unused void *tx_queue, struct
> rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
> index 49c53712a..c4083ff00 100644
> --- a/drivers/net/e1000/em_rxtx.c
> +++ b/drivers/net/e1000/em_rxtx.c
> @@ -626,7 +626,7 @@ eth_em_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
> index 684fa4ad8..6a78f26e6 100644
> --- a/drivers/net/e1000/igb_rxtx.c
> +++ b/drivers/net/e1000/igb_rxtx.c
> @@ -641,7 +641,7 @@ eth_igb_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 665afee4f..b9855e91b 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -2145,7 +2145,7 @@ eth_ena_prep_pkts(void *tx_queue, struct
> rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
> index 6a8718c08..c42d563b4 100644
> --- a/drivers/net/enic/enic_rxtx.c
> +++ b/drivers/net/enic/enic_rxtx.c
> @@ -414,7 +414,7 @@ uint16_t enic_prep_pkts(void *tx_queue, struct
> rte_mbuf **tx_pkts,
> rte_errno = ENOTSUP;
> return i;
> }
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/fm10k/fm10k_rxtx.c
> b/drivers/net/fm10k/fm10k_rxtx.c
> index 4accaa2cd..43d773f08 100644
> --- a/drivers/net/fm10k/fm10k_rxtx.c
> +++ b/drivers/net/fm10k/fm10k_rxtx.c
> @@ -710,7 +710,7 @@ fm10k_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/hinic/hinic_pmd_tx.c
> b/drivers/net/hinic/hinic_pmd_tx.c
> index 64ec2c119..41d5a25b6 100644
> --- a/drivers/net/hinic/hinic_pmd_tx.c
> +++ b/drivers/net/hinic/hinic_pmd_tx.c
> @@ -804,7 +804,7 @@ hinic_tx_offload_pkt_prepare(struct rte_mbuf *m,
> !(ol_flags & PKT_TX_TUNNEL_VXLAN))
> return -ENOTSUP;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> if (rte_validate_tx_offload(m) != 0)
> return -EINVAL;
> #endif
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index ec6d19f58..45aa64b70 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -2296,7 +2296,7 @@ hns3_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 5e7c86ed8..282baf514 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -1499,7 +1499,7 @@ i40e_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index 85d9a8e3b..8122d35be 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -1689,7 +1689,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> index 1c9f31efd..fd8ed2573 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -3037,7 +3037,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index 2e20e18c7..6964c4e52 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -992,7 +992,7 @@ ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf
> **tx_pkts, uint16_t nb_pkts)
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> index b81788ca4..646eb2275 100644
> --- a/drivers/net/qede/qede_rxtx.c
> +++ b/drivers/net/qede/qede_rxtx.c
> @@ -2156,7 +2156,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> struct rte_mbuf **tx_pkts,
> uint64_t ol_flags;
> struct rte_mbuf *m;
> uint16_t i;
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> int ret;
> #endif
>
> @@ -2196,7 +2196,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> struct rte_mbuf **tx_pkts,
> break;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/drivers/net/softnic/rte_eth_softnic.c
> b/drivers/net/softnic/rte_eth_softnic.c
> index 11723778f..b5b169ff7 100644
> --- a/drivers/net/softnic/rte_eth_softnic.c
> +++ b/drivers/net/softnic/rte_eth_softnic.c
> @@ -704,7 +704,7 @@ rte_pmd_softnic_manage(uint16_t port_id)
> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> struct pmd_internals *softnic;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> #endif
>
> diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c
> b/drivers/net/softnic/rte_eth_softnic_thread.c
> index d610b1617..2f7c3a838 100644
> --- a/drivers/net/softnic/rte_eth_softnic_thread.c
> +++ b/drivers/net/softnic/rte_eth_softnic_thread.c
> @@ -3093,7 +3093,7 @@ rte_pmd_softnic_run(uint16_t port_id)
> {
> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> #endif
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 752faa0f6..02eaf38e3 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1979,7 +1979,7 @@ virtio_xmit_pkts_prepare(void *tx_queue
> __rte_unused, struct rte_mbuf **tx_pkts,
> for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
> struct rte_mbuf *m = tx_pkts[nb_tx];
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> error = rte_validate_tx_offload(m);
> if (unlikely(error)) {
> rte_errno = -error;
> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> index dd99684be..a801290ff 100644
> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> @@ -373,7 +373,7 @@ vmxnet3_prep_pkts(__rte_unused void *tx_queue,
> struct rte_mbuf **tx_pkts,
> return i;
> }
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> ret = rte_validate_tx_offload(m);
> if (ret != 0) {
> rte_errno = -ret;
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index e9e3a1699..f314b57c7 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4375,7 +4375,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> queue_id,
> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> uint16_t nb_rx;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
>
> @@ -4498,11 +4498,11 @@ rte_eth_rx_descriptor_status(uint16_t port_id,
> uint16_t queue_id,
> struct rte_eth_dev *dev;
> void *rxq;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> #endif
> dev = &rte_eth_devices[port_id];
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> if (queue_id >= dev->data->nb_rx_queues)
> return -ENODEV;
> #endif
> @@ -4555,11 +4555,11 @@ static inline int
> rte_eth_tx_descriptor_status(uint16_t port_id,
> struct rte_eth_dev *dev;
> void *txq;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> #endif
> dev = &rte_eth_devices[port_id];
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> if (queue_id >= dev->data->nb_tx_queues)
> return -ENODEV;
> #endif
> @@ -4641,7 +4641,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> queue_id,
> {
> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
>
> @@ -4727,7 +4727,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> queue_id,
> {
> struct rte_eth_dev *dev;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> if (!rte_eth_dev_is_valid_port(port_id)) {
> RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
> rte_errno = EINVAL;
> @@ -4737,7 +4737,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> queue_id,
>
> dev = &rte_eth_devices[port_id];
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> if (queue_id >= dev->data->nb_tx_queues) {
> RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> queue_id);
> rte_errno = EINVAL;
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 1560ecfa4..9a9732189 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -120,7 +120,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> *m, uint64_t ol_flags)
> struct rte_udp_hdr *udp_hdr;
> uint64_t inner_l3_offset = m->l2_len;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> /*
> * Does packet set any of available offloads?
> * Mainly it is required to avoid fragmented headers check if
> @@ -133,7 +133,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> *m, uint64_t ol_flags)
> if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
> inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> +#ifdef RTE_DEBUG
> /*
> * Check if headers are fragmented.
> * The check could be less strict depending on which offloads are
> --
> 2.17.1
The approach in this series does not make any sense to me: what if you want to debug just this library instead of the entire DPDK? We need to be able to enable debug for a single library at a time, as opposed to the user being flooded with unwanted debug messages from all the libraries.
NAK from me.
>
>
>
> > -----Original Message-----
> > From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> > Sent: Friday, April 17, 2020 10:57 PM
> > To: Thomas Monjalon <thomas@monjalon.net>; Igor Russkikh
> > <igor.russkikh@aquantia.com>; Pavel Belous <pavel.belous@aquantia.com>;
> > Lu, Wenzhuo <wenzhuo.lu@intel.com>; Marcin Wojtas
> > <mw@semihalf.com>; Michal Krawczyk <mk@semihalf.com>; Guy Tzalik
> > <gtzalik@amazon.com>; Evgeny Schemeilin <evgenys@amazon.com>; Igor
> > Chauskin <igorch@amazon.com>; John Daley <johndale@cisco.com>; Hyong
> > Youb Kim <hyonkim@cisco.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > Wang, Xiao W <xiao.w.wang@intel.com>; Ziyang Xuan
> > <xuanziyang2@huawei.com>; Xiaoyun Wang
> > <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
> > <zhouguoyang@huawei.com>; Wei Hu (Xavier)
> > <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
> > Yisen Zhuang <yisen.zhuang@huawei.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> > <qiming.yang@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Rasesh Mody <rmody@marvell.com>;
> > Shahed Shaikh <shshaikh@marvell.com>; Singh, Jasvinder
> > <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yong
> > Wang <yongwang@vmware.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > <olivier.matz@6wind.com>
> > Cc: dev@dpdk.org
> > Subject: [PATCH v1 03/17] ethdev: replace library debug flag with global one
> >
> > Use global debug flag RTE_DEBUG instead of RTE_LIBRTE_ETHDEV_DEBUG.
> > The old define is completely removed from source code and config.
> > The changes were applied also to all drivers using this flag.
> >
> > Signed-off-by: Lukasz Wojciechowski
> > <l.wojciechow@partner.samsung.com>
> > ---
> > config/common_base | 1 -
> > drivers/net/atlantic/atl_rxtx.c | 2 +-
> > drivers/net/e1000/em_rxtx.c | 2 +-
> > drivers/net/e1000/igb_rxtx.c | 2 +-
> > drivers/net/ena/ena_ethdev.c | 2 +-
> > drivers/net/enic/enic_rxtx.c | 2 +-
> > drivers/net/fm10k/fm10k_rxtx.c | 2 +-
> > drivers/net/hinic/hinic_pmd_tx.c | 2 +-
> > drivers/net/hns3/hns3_rxtx.c | 2 +-
> > drivers/net/i40e/i40e_rxtx.c | 2 +-
> > drivers/net/iavf/iavf_rxtx.c | 2 +-
> > drivers/net/ice/ice_rxtx.c | 2 +-
> > drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
> > drivers/net/qede/qede_rxtx.c | 4 ++--
> > drivers/net/softnic/rte_eth_softnic.c | 2 +-
> > drivers/net/softnic/rte_eth_softnic_thread.c | 2 +-
> > drivers/net/virtio/virtio_rxtx.c | 2 +-
> > drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
> > lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
> > lib/librte_net/rte_net.h | 4 ++--
> > 20 files changed, 28 insertions(+), 29 deletions(-)
> >
> > diff --git a/config/common_base b/config/common_base
> > index c5be57f11..16a8f09b6 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -149,7 +149,6 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> > # Compile generic ethernet library
> > #
> > CONFIG_RTE_LIBRTE_ETHER=y
> > -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> > CONFIG_RTE_MAX_ETHPORTS=32
> > CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > CONFIG_RTE_LIBRTE_IEEE1588=n
> > diff --git a/drivers/net/atlantic/atl_rxtx.c b/drivers/net/atlantic/atl_rxtx.c
> > index 449ffd454..eae54df22 100644
> > --- a/drivers/net/atlantic/atl_rxtx.c
> > +++ b/drivers/net/atlantic/atl_rxtx.c
> > @@ -821,7 +821,7 @@ atl_prep_pkts(__rte_unused void *tx_queue, struct
> > rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
> > index 49c53712a..c4083ff00 100644
> > --- a/drivers/net/e1000/em_rxtx.c
> > +++ b/drivers/net/e1000/em_rxtx.c
> > @@ -626,7 +626,7 @@ eth_em_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
> > index 684fa4ad8..6a78f26e6 100644
> > --- a/drivers/net/e1000/igb_rxtx.c
> > +++ b/drivers/net/e1000/igb_rxtx.c
> > @@ -641,7 +641,7 @@ eth_igb_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > index 665afee4f..b9855e91b 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -2145,7 +2145,7 @@ eth_ena_prep_pkts(void *tx_queue, struct
> > rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
> > index 6a8718c08..c42d563b4 100644
> > --- a/drivers/net/enic/enic_rxtx.c
> > +++ b/drivers/net/enic/enic_rxtx.c
> > @@ -414,7 +414,7 @@ uint16_t enic_prep_pkts(void *tx_queue, struct
> > rte_mbuf **tx_pkts,
> > rte_errno = ENOTSUP;
> > return i;
> > }
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/fm10k/fm10k_rxtx.c
> > b/drivers/net/fm10k/fm10k_rxtx.c
> > index 4accaa2cd..43d773f08 100644
> > --- a/drivers/net/fm10k/fm10k_rxtx.c
> > +++ b/drivers/net/fm10k/fm10k_rxtx.c
> > @@ -710,7 +710,7 @@ fm10k_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/hinic/hinic_pmd_tx.c
> > b/drivers/net/hinic/hinic_pmd_tx.c
> > index 64ec2c119..41d5a25b6 100644
> > --- a/drivers/net/hinic/hinic_pmd_tx.c
> > +++ b/drivers/net/hinic/hinic_pmd_tx.c
> > @@ -804,7 +804,7 @@ hinic_tx_offload_pkt_prepare(struct rte_mbuf *m,
> > !(ol_flags & PKT_TX_TUNNEL_VXLAN))
> > return -ENOTSUP;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > if (rte_validate_tx_offload(m) != 0)
> > return -EINVAL;
> > #endif
> > diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> > index ec6d19f58..45aa64b70 100644
> > --- a/drivers/net/hns3/hns3_rxtx.c
> > +++ b/drivers/net/hns3/hns3_rxtx.c
> > @@ -2296,7 +2296,7 @@ hns3_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > index 5e7c86ed8..282baf514 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -1499,7 +1499,7 @@ i40e_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> > index 85d9a8e3b..8122d35be 100644
> > --- a/drivers/net/iavf/iavf_rxtx.c
> > +++ b/drivers/net/iavf/iavf_rxtx.c
> > @@ -1689,7 +1689,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > index 1c9f31efd..fd8ed2573 100644
> > --- a/drivers/net/ice/ice_rxtx.c
> > +++ b/drivers/net/ice/ice_rxtx.c
> > @@ -3037,7 +3037,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> > index 2e20e18c7..6964c4e52 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -992,7 +992,7 @@ ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf
> > **tx_pkts, uint16_t nb_pkts)
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> > index b81788ca4..646eb2275 100644
> > --- a/drivers/net/qede/qede_rxtx.c
> > +++ b/drivers/net/qede/qede_rxtx.c
> > @@ -2156,7 +2156,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> > struct rte_mbuf **tx_pkts,
> > uint64_t ol_flags;
> > struct rte_mbuf *m;
> > uint16_t i;
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > int ret;
> > #endif
> >
> > @@ -2196,7 +2196,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> > struct rte_mbuf **tx_pkts,
> > break;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/drivers/net/softnic/rte_eth_softnic.c
> > b/drivers/net/softnic/rte_eth_softnic.c
> > index 11723778f..b5b169ff7 100644
> > --- a/drivers/net/softnic/rte_eth_softnic.c
> > +++ b/drivers/net/softnic/rte_eth_softnic.c
> > @@ -704,7 +704,7 @@ rte_pmd_softnic_manage(uint16_t port_id)
> > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > struct pmd_internals *softnic;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > #endif
> >
> > diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c
> > b/drivers/net/softnic/rte_eth_softnic_thread.c
> > index d610b1617..2f7c3a838 100644
> > --- a/drivers/net/softnic/rte_eth_softnic_thread.c
> > +++ b/drivers/net/softnic/rte_eth_softnic_thread.c
> > @@ -3093,7 +3093,7 @@ rte_pmd_softnic_run(uint16_t port_id)
> > {
> > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > #endif
> >
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 752faa0f6..02eaf38e3 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -1979,7 +1979,7 @@ virtio_xmit_pkts_prepare(void *tx_queue
> > __rte_unused, struct rte_mbuf **tx_pkts,
> > for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
> > struct rte_mbuf *m = tx_pkts[nb_tx];
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > error = rte_validate_tx_offload(m);
> > if (unlikely(error)) {
> > rte_errno = -error;
> > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > index dd99684be..a801290ff 100644
> > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > @@ -373,7 +373,7 @@ vmxnet3_prep_pkts(__rte_unused void *tx_queue,
> > struct rte_mbuf **tx_pkts,
> > return i;
> > }
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > ret = rte_validate_tx_offload(m);
> > if (ret != 0) {
> > rte_errno = -ret;
> > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > index e9e3a1699..f314b57c7 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -4375,7 +4375,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> > queue_id,
> > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > uint16_t nb_rx;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> >
> > @@ -4498,11 +4498,11 @@ rte_eth_rx_descriptor_status(uint16_t port_id,
> > uint16_t queue_id,
> > struct rte_eth_dev *dev;
> > void *rxq;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > #endif
> > dev = &rte_eth_devices[port_id];
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > if (queue_id >= dev->data->nb_rx_queues)
> > return -ENODEV;
> > #endif
> > @@ -4555,11 +4555,11 @@ static inline int
> > rte_eth_tx_descriptor_status(uint16_t port_id,
> > struct rte_eth_dev *dev;
> > void *txq;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > #endif
> > dev = &rte_eth_devices[port_id];
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > if (queue_id >= dev->data->nb_tx_queues)
> > return -ENODEV;
> > #endif
> > @@ -4641,7 +4641,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> > queue_id,
> > {
> > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
> >
> > @@ -4727,7 +4727,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> > queue_id,
> > {
> > struct rte_eth_dev *dev;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > if (!rte_eth_dev_is_valid_port(port_id)) {
> > RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
> > rte_errno = EINVAL;
> > @@ -4737,7 +4737,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> > queue_id,
> >
> > dev = &rte_eth_devices[port_id];
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > if (queue_id >= dev->data->nb_tx_queues) {
> > RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> > queue_id);
> > rte_errno = EINVAL;
> > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> > index 1560ecfa4..9a9732189 100644
> > --- a/lib/librte_net/rte_net.h
> > +++ b/lib/librte_net/rte_net.h
> > @@ -120,7 +120,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> > *m, uint64_t ol_flags)
> > struct rte_udp_hdr *udp_hdr;
> > uint64_t inner_l3_offset = m->l2_len;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > /*
> > * Does packet set any of available offloads?
> > * Mainly it is required to avoid fragmented headers check if
> > @@ -133,7 +133,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> > *m, uint64_t ol_flags)
> > if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
> > inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> >
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > +#ifdef RTE_DEBUG
> > /*
> > * Check if headers are fragmented.
> > * The check could be less strict depending on which offloads are
> > --
> > 2.17.1
>
> The approach in this series does not make any sense to me: what if you want to debug just this library instead of the entire DPDK? We need
> to be able to enable debug for a single library at a time, as opposed to the user being flooded with unwanted debug messages from all the
> libraries.
>
> NAK from me.
I am agree with Cristian concern here:
that patch removes ability to enable/disable debug on particular library/PMD.
If the purpose is to minimize number of config compile options,
I wonder can't it be done in a slightly different way:
1. introduce gloabal RTE_DEBUG
2. keep actual .[c,h] files intact
3. In actual librte_xxx/meson.build file check if RTE_DEBUG is enabled,
If yes, then enable particular debug flag for these libs.
Something like:
If dpdk_conf.get('RTE_DEBUG') == true
dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
defines that are used by multiple libs, probably can be
set in upper layer meson.build.
That way will have global 'debug' flag, but users will still
have an ability to enable/disable debug flags on a per lib basis
via CFLAGS="-D..."
Konstantin
On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> > > Sent: Friday, April 17, 2020 10:57 PM
> > > To: Thomas Monjalon <thomas@monjalon.net>; Igor Russkikh
> > > <igor.russkikh@aquantia.com>; Pavel Belous <pavel.belous@aquantia.com>;
> > > Lu, Wenzhuo <wenzhuo.lu@intel.com>; Marcin Wojtas
> > > <mw@semihalf.com>; Michal Krawczyk <mk@semihalf.com>; Guy Tzalik
> > > <gtzalik@amazon.com>; Evgeny Schemeilin <evgenys@amazon.com>; Igor
> > > Chauskin <igorch@amazon.com>; John Daley <johndale@cisco.com>; Hyong
> > > Youb Kim <hyonkim@cisco.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > > Wang, Xiao W <xiao.w.wang@intel.com>; Ziyang Xuan
> > > <xuanziyang2@huawei.com>; Xiaoyun Wang
> > > <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
> > > <zhouguoyang@huawei.com>; Wei Hu (Xavier)
> > > <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
> > > Yisen Zhuang <yisen.zhuang@huawei.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> > > <qiming.yang@intel.com>; Ananyev, Konstantin
> > > <konstantin.ananyev@intel.com>; Rasesh Mody <rmody@marvell.com>;
> > > Shahed Shaikh <shshaikh@marvell.com>; Singh, Jasvinder
> > > <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yong
> > > Wang <yongwang@vmware.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > > Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
> > > <olivier.matz@6wind.com>
> > > Cc: dev@dpdk.org
> > > Subject: [PATCH v1 03/17] ethdev: replace library debug flag with global one
> > >
> > > Use global debug flag RTE_DEBUG instead of RTE_LIBRTE_ETHDEV_DEBUG.
> > > The old define is completely removed from source code and config.
> > > The changes were applied also to all drivers using this flag.
> > >
> > > Signed-off-by: Lukasz Wojciechowski
> > > <l.wojciechow@partner.samsung.com>
> > > ---
> > > config/common_base | 1 -
> > > drivers/net/atlantic/atl_rxtx.c | 2 +-
> > > drivers/net/e1000/em_rxtx.c | 2 +-
> > > drivers/net/e1000/igb_rxtx.c | 2 +-
> > > drivers/net/ena/ena_ethdev.c | 2 +-
> > > drivers/net/enic/enic_rxtx.c | 2 +-
> > > drivers/net/fm10k/fm10k_rxtx.c | 2 +-
> > > drivers/net/hinic/hinic_pmd_tx.c | 2 +-
> > > drivers/net/hns3/hns3_rxtx.c | 2 +-
> > > drivers/net/i40e/i40e_rxtx.c | 2 +-
> > > drivers/net/iavf/iavf_rxtx.c | 2 +-
> > > drivers/net/ice/ice_rxtx.c | 2 +-
> > > drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
> > > drivers/net/qede/qede_rxtx.c | 4 ++--
> > > drivers/net/softnic/rte_eth_softnic.c | 2 +-
> > > drivers/net/softnic/rte_eth_softnic_thread.c | 2 +-
> > > drivers/net/virtio/virtio_rxtx.c | 2 +-
> > > drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
> > > lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
> > > lib/librte_net/rte_net.h | 4 ++--
> > > 20 files changed, 28 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/config/common_base b/config/common_base
> > > index c5be57f11..16a8f09b6 100644
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > > @@ -149,7 +149,6 @@ CONFIG_RTE_LIBRTE_KVARGS=y
> > > # Compile generic ethernet library
> > > #
> > > CONFIG_RTE_LIBRTE_ETHER=y
> > > -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> > > CONFIG_RTE_MAX_ETHPORTS=32
> > > CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > CONFIG_RTE_LIBRTE_IEEE1588=n
> > > diff --git a/drivers/net/atlantic/atl_rxtx.c b/drivers/net/atlantic/atl_rxtx.c
> > > index 449ffd454..eae54df22 100644
> > > --- a/drivers/net/atlantic/atl_rxtx.c
> > > +++ b/drivers/net/atlantic/atl_rxtx.c
> > > @@ -821,7 +821,7 @@ atl_prep_pkts(__rte_unused void *tx_queue, struct
> > > rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
> > > index 49c53712a..c4083ff00 100644
> > > --- a/drivers/net/e1000/em_rxtx.c
> > > +++ b/drivers/net/e1000/em_rxtx.c
> > > @@ -626,7 +626,7 @@ eth_em_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
> > > index 684fa4ad8..6a78f26e6 100644
> > > --- a/drivers/net/e1000/igb_rxtx.c
> > > +++ b/drivers/net/e1000/igb_rxtx.c
> > > @@ -641,7 +641,7 @@ eth_igb_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > > index 665afee4f..b9855e91b 100644
> > > --- a/drivers/net/ena/ena_ethdev.c
> > > +++ b/drivers/net/ena/ena_ethdev.c
> > > @@ -2145,7 +2145,7 @@ eth_ena_prep_pkts(void *tx_queue, struct
> > > rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
> > > index 6a8718c08..c42d563b4 100644
> > > --- a/drivers/net/enic/enic_rxtx.c
> > > +++ b/drivers/net/enic/enic_rxtx.c
> > > @@ -414,7 +414,7 @@ uint16_t enic_prep_pkts(void *tx_queue, struct
> > > rte_mbuf **tx_pkts,
> > > rte_errno = ENOTSUP;
> > > return i;
> > > }
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/fm10k/fm10k_rxtx.c
> > > b/drivers/net/fm10k/fm10k_rxtx.c
> > > index 4accaa2cd..43d773f08 100644
> > > --- a/drivers/net/fm10k/fm10k_rxtx.c
> > > +++ b/drivers/net/fm10k/fm10k_rxtx.c
> > > @@ -710,7 +710,7 @@ fm10k_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/hinic/hinic_pmd_tx.c
> > > b/drivers/net/hinic/hinic_pmd_tx.c
> > > index 64ec2c119..41d5a25b6 100644
> > > --- a/drivers/net/hinic/hinic_pmd_tx.c
> > > +++ b/drivers/net/hinic/hinic_pmd_tx.c
> > > @@ -804,7 +804,7 @@ hinic_tx_offload_pkt_prepare(struct rte_mbuf *m,
> > > !(ol_flags & PKT_TX_TUNNEL_VXLAN))
> > > return -ENOTSUP;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > if (rte_validate_tx_offload(m) != 0)
> > > return -EINVAL;
> > > #endif
> > > diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> > > index ec6d19f58..45aa64b70 100644
> > > --- a/drivers/net/hns3/hns3_rxtx.c
> > > +++ b/drivers/net/hns3/hns3_rxtx.c
> > > @@ -2296,7 +2296,7 @@ hns3_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> > > index 5e7c86ed8..282baf514 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -1499,7 +1499,7 @@ i40e_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> > > index 85d9a8e3b..8122d35be 100644
> > > --- a/drivers/net/iavf/iavf_rxtx.c
> > > +++ b/drivers/net/iavf/iavf_rxtx.c
> > > @@ -1689,7 +1689,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> > > index 1c9f31efd..fd8ed2573 100644
> > > --- a/drivers/net/ice/ice_rxtx.c
> > > +++ b/drivers/net/ice/ice_rxtx.c
> > > @@ -3037,7 +3037,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > index 2e20e18c7..6964c4e52 100644
> > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > @@ -992,7 +992,7 @@ ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf
> > > **tx_pkts, uint16_t nb_pkts)
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
> > > index b81788ca4..646eb2275 100644
> > > --- a/drivers/net/qede/qede_rxtx.c
> > > +++ b/drivers/net/qede/qede_rxtx.c
> > > @@ -2156,7 +2156,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> > > struct rte_mbuf **tx_pkts,
> > > uint64_t ol_flags;
> > > struct rte_mbuf *m;
> > > uint16_t i;
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > int ret;
> > > #endif
> > >
> > > @@ -2196,7 +2196,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
> > > struct rte_mbuf **tx_pkts,
> > > break;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/drivers/net/softnic/rte_eth_softnic.c
> > > b/drivers/net/softnic/rte_eth_softnic.c
> > > index 11723778f..b5b169ff7 100644
> > > --- a/drivers/net/softnic/rte_eth_softnic.c
> > > +++ b/drivers/net/softnic/rte_eth_softnic.c
> > > @@ -704,7 +704,7 @@ rte_pmd_softnic_manage(uint16_t port_id)
> > > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > struct pmd_internals *softnic;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > #endif
> > >
> > > diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c
> > > b/drivers/net/softnic/rte_eth_softnic_thread.c
> > > index d610b1617..2f7c3a838 100644
> > > --- a/drivers/net/softnic/rte_eth_softnic_thread.c
> > > +++ b/drivers/net/softnic/rte_eth_softnic_thread.c
> > > @@ -3093,7 +3093,7 @@ rte_pmd_softnic_run(uint16_t port_id)
> > > {
> > > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > #endif
> > >
> > > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > > index 752faa0f6..02eaf38e3 100644
> > > --- a/drivers/net/virtio/virtio_rxtx.c
> > > +++ b/drivers/net/virtio/virtio_rxtx.c
> > > @@ -1979,7 +1979,7 @@ virtio_xmit_pkts_prepare(void *tx_queue
> > > __rte_unused, struct rte_mbuf **tx_pkts,
> > > for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
> > > struct rte_mbuf *m = tx_pkts[nb_tx];
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > error = rte_validate_tx_offload(m);
> > > if (unlikely(error)) {
> > > rte_errno = -error;
> > > diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > > b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > > index dd99684be..a801290ff 100644
> > > --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > > +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
> > > @@ -373,7 +373,7 @@ vmxnet3_prep_pkts(__rte_unused void *tx_queue,
> > > struct rte_mbuf **tx_pkts,
> > > return i;
> > > }
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > ret = rte_validate_tx_offload(m);
> > > if (ret != 0) {
> > > rte_errno = -ret;
> > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> > > index e9e3a1699..f314b57c7 100644
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -4375,7 +4375,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
> > > queue_id,
> > > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > > uint16_t nb_rx;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
> > >
> > > @@ -4498,11 +4498,11 @@ rte_eth_rx_descriptor_status(uint16_t port_id,
> > > uint16_t queue_id,
> > > struct rte_eth_dev *dev;
> > > void *rxq;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > #endif
> > > dev = &rte_eth_devices[port_id];
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > if (queue_id >= dev->data->nb_rx_queues)
> > > return -ENODEV;
> > > #endif
> > > @@ -4555,11 +4555,11 @@ static inline int
> > > rte_eth_tx_descriptor_status(uint16_t port_id,
> > > struct rte_eth_dev *dev;
> > > void *txq;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > #endif
> > > dev = &rte_eth_devices[port_id];
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > if (queue_id >= dev->data->nb_tx_queues)
> > > return -ENODEV;
> > > #endif
> > > @@ -4641,7 +4641,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
> > > queue_id,
> > > {
> > > struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
> > >
> > > @@ -4727,7 +4727,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> > > queue_id,
> > > {
> > > struct rte_eth_dev *dev;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > if (!rte_eth_dev_is_valid_port(port_id)) {
> > > RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
> > > rte_errno = EINVAL;
> > > @@ -4737,7 +4737,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
> > > queue_id,
> > >
> > > dev = &rte_eth_devices[port_id];
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > if (queue_id >= dev->data->nb_tx_queues) {
> > > RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
> > > queue_id);
> > > rte_errno = EINVAL;
> > > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> > > index 1560ecfa4..9a9732189 100644
> > > --- a/lib/librte_net/rte_net.h
> > > +++ b/lib/librte_net/rte_net.h
> > > @@ -120,7 +120,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> > > *m, uint64_t ol_flags)
> > > struct rte_udp_hdr *udp_hdr;
> > > uint64_t inner_l3_offset = m->l2_len;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > /*
> > > * Does packet set any of available offloads?
> > > * Mainly it is required to avoid fragmented headers check if
> > > @@ -133,7 +133,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
> > > *m, uint64_t ol_flags)
> > > if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
> > > inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > >
> > > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > > +#ifdef RTE_DEBUG
> > > /*
> > > * Check if headers are fragmented.
> > > * The check could be less strict depending on which offloads are
> > > --
> > > 2.17.1
> >
> > The approach in this series does not make any sense to me: what if you want to debug just this library instead of the entire DPDK? We need
> > to be able to enable debug for a single library at a time, as opposed to the user being flooded with unwanted debug messages from all the
> > libraries.
> >
> > NAK from me.
>
> I am agree with Cristian concern here:
> that patch removes ability to enable/disable debug on particular library/PMD.
> If the purpose is to minimize number of config compile options,
> I wonder can't it be done in a slightly different way:
> 1. introduce gloabal RTE_DEBUG
> 2. keep actual .[c,h] files intact
> 3. In actual librte_xxx/meson.build file check if RTE_DEBUG is enabled,
> If yes, then enable particular debug flag for these libs.
> Something like:
> If dpdk_conf.get('RTE_DEBUG') == true
> dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>
> defines that are used by multiple libs, probably can be
> set in upper layer meson.build.
>
> That way will have global 'debug' flag, but users will still
> have an ability to enable/disable debug flags on a per lib basis
> via CFLAGS="-D..."
>
> Konstantin
>
That seems a reasonable idea to me.
However, in this case, we don't need the RTE_DEBUG flag at all, we can
either:
* allow each component meson.build file define its own flags after checking
get_option('debug')
* have lib/meson.build and drivers/meson.build automatically define a
specific define for each library or driver to standardize the naming.
[This would save anyone working on it from having to lookup what the
define was, since it's always e.g. RTE_DEBUG_ + library-base-name, e.g.
RTE_DEBUG_LPM, RTE_DEBUG_SCHED etc]
Theoretically we can also do both, have the standard ones defined and then
allow a component to provide extra flags itself if so desired.
/Bruce
W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>>>> Sent: Friday, April 17, 2020 10:57 PM
>>>> To: Thomas Monjalon <thomas@monjalon.net>; Igor Russkikh
>>>> <igor.russkikh@aquantia.com>; Pavel Belous <pavel.belous@aquantia.com>;
>>>> Lu, Wenzhuo <wenzhuo.lu@intel.com>; Marcin Wojtas
>>>> <mw@semihalf.com>; Michal Krawczyk <mk@semihalf.com>; Guy Tzalik
>>>> <gtzalik@amazon.com>; Evgeny Schemeilin <evgenys@amazon.com>; Igor
>>>> Chauskin <igorch@amazon.com>; John Daley <johndale@cisco.com>; Hyong
>>>> Youb Kim <hyonkim@cisco.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>>>> Wang, Xiao W <xiao.w.wang@intel.com>; Ziyang Xuan
>>>> <xuanziyang2@huawei.com>; Xiaoyun Wang
>>>> <cloud.wangxiaoyun@huawei.com>; Guoyang Zhou
>>>> <zhouguoyang@huawei.com>; Wei Hu (Xavier)
>>>> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
>>>> Yisen Zhuang <yisen.zhuang@huawei.com>; Xing, Beilei
>>>> <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
>>>> <qiming.yang@intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev@intel.com>; Rasesh Mody <rmody@marvell.com>;
>>>> Shahed Shaikh <shshaikh@marvell.com>; Singh, Jasvinder
>>>> <jasvinder.singh@intel.com>; Dumitrescu, Cristian
>>>> <cristian.dumitrescu@intel.com>; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Yong
>>>> Wang <yongwang@vmware.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
>>>> Andrew Rybchenko <arybchenko@solarflare.com>; Olivier Matz
>>>> <olivier.matz@6wind.com>
>>>> Cc: dev@dpdk.org
>>>> Subject: [PATCH v1 03/17] ethdev: replace library debug flag with global one
>>>>
>>>> Use global debug flag RTE_DEBUG instead of RTE_LIBRTE_ETHDEV_DEBUG.
>>>> The old define is completely removed from source code and config.
>>>> The changes were applied also to all drivers using this flag.
>>>>
>>>> Signed-off-by: Lukasz Wojciechowski
>>>> <l.wojciechow@partner.samsung.com>
>>>> ---
>>>> config/common_base | 1 -
>>>> drivers/net/atlantic/atl_rxtx.c | 2 +-
>>>> drivers/net/e1000/em_rxtx.c | 2 +-
>>>> drivers/net/e1000/igb_rxtx.c | 2 +-
>>>> drivers/net/ena/ena_ethdev.c | 2 +-
>>>> drivers/net/enic/enic_rxtx.c | 2 +-
>>>> drivers/net/fm10k/fm10k_rxtx.c | 2 +-
>>>> drivers/net/hinic/hinic_pmd_tx.c | 2 +-
>>>> drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>> drivers/net/i40e/i40e_rxtx.c | 2 +-
>>>> drivers/net/iavf/iavf_rxtx.c | 2 +-
>>>> drivers/net/ice/ice_rxtx.c | 2 +-
>>>> drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
>>>> drivers/net/qede/qede_rxtx.c | 4 ++--
>>>> drivers/net/softnic/rte_eth_softnic.c | 2 +-
>>>> drivers/net/softnic/rte_eth_softnic_thread.c | 2 +-
>>>> drivers/net/virtio/virtio_rxtx.c | 2 +-
>>>> drivers/net/vmxnet3/vmxnet3_rxtx.c | 2 +-
>>>> lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
>>>> lib/librte_net/rte_net.h | 4 ++--
>>>> 20 files changed, 28 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/config/common_base b/config/common_base
>>>> index c5be57f11..16a8f09b6 100644
>>>> --- a/config/common_base
>>>> +++ b/config/common_base
>>>> @@ -149,7 +149,6 @@ CONFIG_RTE_LIBRTE_KVARGS=y
>>>> # Compile generic ethernet library
>>>> #
>>>> CONFIG_RTE_LIBRTE_ETHER=y
>>>> -CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
>>>> CONFIG_RTE_MAX_ETHPORTS=32
>>>> CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
>>>> CONFIG_RTE_LIBRTE_IEEE1588=n
>>>> diff --git a/drivers/net/atlantic/atl_rxtx.c b/drivers/net/atlantic/atl_rxtx.c
>>>> index 449ffd454..eae54df22 100644
>>>> --- a/drivers/net/atlantic/atl_rxtx.c
>>>> +++ b/drivers/net/atlantic/atl_rxtx.c
>>>> @@ -821,7 +821,7 @@ atl_prep_pkts(__rte_unused void *tx_queue, struct
>>>> rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
>>>> index 49c53712a..c4083ff00 100644
>>>> --- a/drivers/net/e1000/em_rxtx.c
>>>> +++ b/drivers/net/e1000/em_rxtx.c
>>>> @@ -626,7 +626,7 @@ eth_em_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
>>>> index 684fa4ad8..6a78f26e6 100644
>>>> --- a/drivers/net/e1000/igb_rxtx.c
>>>> +++ b/drivers/net/e1000/igb_rxtx.c
>>>> @@ -641,7 +641,7 @@ eth_igb_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
>>>> index 665afee4f..b9855e91b 100644
>>>> --- a/drivers/net/ena/ena_ethdev.c
>>>> +++ b/drivers/net/ena/ena_ethdev.c
>>>> @@ -2145,7 +2145,7 @@ eth_ena_prep_pkts(void *tx_queue, struct
>>>> rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/enic/enic_rxtx.c b/drivers/net/enic/enic_rxtx.c
>>>> index 6a8718c08..c42d563b4 100644
>>>> --- a/drivers/net/enic/enic_rxtx.c
>>>> +++ b/drivers/net/enic/enic_rxtx.c
>>>> @@ -414,7 +414,7 @@ uint16_t enic_prep_pkts(void *tx_queue, struct
>>>> rte_mbuf **tx_pkts,
>>>> rte_errno = ENOTSUP;
>>>> return i;
>>>> }
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/fm10k/fm10k_rxtx.c
>>>> b/drivers/net/fm10k/fm10k_rxtx.c
>>>> index 4accaa2cd..43d773f08 100644
>>>> --- a/drivers/net/fm10k/fm10k_rxtx.c
>>>> +++ b/drivers/net/fm10k/fm10k_rxtx.c
>>>> @@ -710,7 +710,7 @@ fm10k_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/hinic/hinic_pmd_tx.c
>>>> b/drivers/net/hinic/hinic_pmd_tx.c
>>>> index 64ec2c119..41d5a25b6 100644
>>>> --- a/drivers/net/hinic/hinic_pmd_tx.c
>>>> +++ b/drivers/net/hinic/hinic_pmd_tx.c
>>>> @@ -804,7 +804,7 @@ hinic_tx_offload_pkt_prepare(struct rte_mbuf *m,
>>>> !(ol_flags & PKT_TX_TUNNEL_VXLAN))
>>>> return -ENOTSUP;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> if (rte_validate_tx_offload(m) != 0)
>>>> return -EINVAL;
>>>> #endif
>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>>>> index ec6d19f58..45aa64b70 100644
>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>> @@ -2296,7 +2296,7 @@ hns3_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
>>>> index 5e7c86ed8..282baf514 100644
>>>> --- a/drivers/net/i40e/i40e_rxtx.c
>>>> +++ b/drivers/net/i40e/i40e_rxtx.c
>>>> @@ -1499,7 +1499,7 @@ i40e_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
>>>> index 85d9a8e3b..8122d35be 100644
>>>> --- a/drivers/net/iavf/iavf_rxtx.c
>>>> +++ b/drivers/net/iavf/iavf_rxtx.c
>>>> @@ -1689,7 +1689,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
>>>> index 1c9f31efd..fd8ed2573 100644
>>>> --- a/drivers/net/ice/ice_rxtx.c
>>>> +++ b/drivers/net/ice/ice_rxtx.c
>>>> @@ -3037,7 +3037,7 @@ ice_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>>>> index 2e20e18c7..6964c4e52 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>>>> @@ -992,7 +992,7 @@ ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf
>>>> **tx_pkts, uint16_t nb_pkts)
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
>>>> index b81788ca4..646eb2275 100644
>>>> --- a/drivers/net/qede/qede_rxtx.c
>>>> +++ b/drivers/net/qede/qede_rxtx.c
>>>> @@ -2156,7 +2156,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
>>>> struct rte_mbuf **tx_pkts,
>>>> uint64_t ol_flags;
>>>> struct rte_mbuf *m;
>>>> uint16_t i;
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> int ret;
>>>> #endif
>>>>
>>>> @@ -2196,7 +2196,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq,
>>>> struct rte_mbuf **tx_pkts,
>>>> break;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/drivers/net/softnic/rte_eth_softnic.c
>>>> b/drivers/net/softnic/rte_eth_softnic.c
>>>> index 11723778f..b5b169ff7 100644
>>>> --- a/drivers/net/softnic/rte_eth_softnic.c
>>>> +++ b/drivers/net/softnic/rte_eth_softnic.c
>>>> @@ -704,7 +704,7 @@ rte_pmd_softnic_manage(uint16_t port_id)
>>>> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>> struct pmd_internals *softnic;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>>> #endif
>>>>
>>>> diff --git a/drivers/net/softnic/rte_eth_softnic_thread.c
>>>> b/drivers/net/softnic/rte_eth_softnic_thread.c
>>>> index d610b1617..2f7c3a838 100644
>>>> --- a/drivers/net/softnic/rte_eth_softnic_thread.c
>>>> +++ b/drivers/net/softnic/rte_eth_softnic_thread.c
>>>> @@ -3093,7 +3093,7 @@ rte_pmd_softnic_run(uint16_t port_id)
>>>> {
>>>> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>>> #endif
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>>> index 752faa0f6..02eaf38e3 100644
>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>> @@ -1979,7 +1979,7 @@ virtio_xmit_pkts_prepare(void *tx_queue
>>>> __rte_unused, struct rte_mbuf **tx_pkts,
>>>> for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
>>>> struct rte_mbuf *m = tx_pkts[nb_tx];
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> error = rte_validate_tx_offload(m);
>>>> if (unlikely(error)) {
>>>> rte_errno = -error;
>>>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> index dd99684be..a801290ff 100644
>>>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>>>> @@ -373,7 +373,7 @@ vmxnet3_prep_pkts(__rte_unused void *tx_queue,
>>>> struct rte_mbuf **tx_pkts,
>>>> return i;
>>>> }
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> ret = rte_validate_tx_offload(m);
>>>> if (ret != 0) {
>>>> rte_errno = -ret;
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>> index e9e3a1699..f314b57c7 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -4375,7 +4375,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
>>>> queue_id,
>>>> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>> uint16_t nb_rx;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
>>>>
>>>> @@ -4498,11 +4498,11 @@ rte_eth_rx_descriptor_status(uint16_t port_id,
>>>> uint16_t queue_id,
>>>> struct rte_eth_dev *dev;
>>>> void *rxq;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>> #endif
>>>> dev = &rte_eth_devices[port_id];
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> if (queue_id >= dev->data->nb_rx_queues)
>>>> return -ENODEV;
>>>> #endif
>>>> @@ -4555,11 +4555,11 @@ static inline int
>>>> rte_eth_tx_descriptor_status(uint16_t port_id,
>>>> struct rte_eth_dev *dev;
>>>> void *txq;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>> #endif
>>>> dev = &rte_eth_devices[port_id];
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> if (queue_id >= dev->data->nb_tx_queues)
>>>> return -ENODEV;
>>>> #endif
>>>> @@ -4641,7 +4641,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
>>>> queue_id,
>>>> {
>>>> struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
>>>>
>>>> @@ -4727,7 +4727,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
>>>> queue_id,
>>>> {
>>>> struct rte_eth_dev *dev;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> if (!rte_eth_dev_is_valid_port(port_id)) {
>>>> RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
>>>> rte_errno = EINVAL;
>>>> @@ -4737,7 +4737,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
>>>> queue_id,
>>>>
>>>> dev = &rte_eth_devices[port_id];
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> if (queue_id >= dev->data->nb_tx_queues) {
>>>> RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
>>>> queue_id);
>>>> rte_errno = EINVAL;
>>>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>>>> index 1560ecfa4..9a9732189 100644
>>>> --- a/lib/librte_net/rte_net.h
>>>> +++ b/lib/librte_net/rte_net.h
>>>> @@ -120,7 +120,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
>>>> *m, uint64_t ol_flags)
>>>> struct rte_udp_hdr *udp_hdr;
>>>> uint64_t inner_l3_offset = m->l2_len;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> /*
>>>> * Does packet set any of available offloads?
>>>> * Mainly it is required to avoid fragmented headers check if
>>>> @@ -133,7 +133,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf
>>>> *m, uint64_t ol_flags)
>>>> if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
>>>> inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>>>
>>>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>>> +#ifdef RTE_DEBUG
>>>> /*
>>>> * Check if headers are fragmented.
>>>> * The check could be less strict depending on which offloads are
>>>> --
>>>> 2.17.1
>>> The approach in this series does not make any sense to me: what if you want to debug just this library instead of the entire DPDK? We need
>>> to be able to enable debug for a single library at a time, as opposed to the user being flooded with unwanted debug messages from all the
>>> libraries.
>>>
>>> NAK from me.
>> I am agree with Cristian concern here:
>> that patch removes ability to enable/disable debug on particular library/PMD.
>> If the purpose is to minimize number of config compile options,
>> I wonder can't it be done in a slightly different way:
>> 1. introduce gloabal RTE_DEBUG
>> 2. keep actual .[c,h] files intact
>> 3. In actual librte_xxx/meson.build file check if RTE_DEBUG is enabled,
>> If yes, then enable particular debug flag for these libs.
>> Something like:
>> If dpdk_conf.get('RTE_DEBUG') == true
>> dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>
>> defines that are used by multiple libs, probably can be
>> set in upper layer meson.build.
>>
>> That way will have global 'debug' flag, but users will still
>> have an ability to enable/disable debug flags on a per lib basis
>> via CFLAGS="-D..."
>>
>> Konstantin
>>
> That seems a reasonable idea to me.
>
> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> either:
>
> * allow each component meson.build file define its own flags after checking
> get_option('debug')
> * have lib/meson.build and drivers/meson.build automatically define a
> specific define for each library or driver to standardize the naming.
> [This would save anyone working on it from having to lookup what the
> define was, since it's always e.g. RTE_DEBUG_ + library-base-name, e.g.
> RTE_DEBUG_LPM, RTE_DEBUG_SCHED etc]
>
> Theoretically we can also do both, have the standard ones defined and then
> allow a component to provide extra flags itself if so desired.
>
> /Bruce
OK, so let's summarize how the patches should be redo:
* usage of global "debug" flag for meson build stays
* we standardize names of debug flags in the components to RTE_DEBUG_ +
components name
* debug flag enables al the RTE_DEBUG_... flags
This allow to easily use both:
* the debug flag - to enable all debugs
* or define manually RTE_DEBUG+component name, just for debug from a
single component
I like Bruce's idea of adding it to the lib/meson.build and
drivers/meson.build. This way they will be added to dpdk_conf meson
object and written then later to rte_build_config.h before compilation
stage.
All the other modules will be able to use these flags.
If there are no objections for such solution, I'll prepare v2
containing above changes.
On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
>
> W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> >>>
<snip>
> >> I am agree with Cristian concern here: that patch removes ability to
> >> enable/disable debug on particular library/PMD. If the purpose is to
> >> minimize number of config compile options, I wonder can't it be done
> >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> >>
> >> defines that are used by multiple libs, probably can be set in upper
> >> layer meson.build.
> >>
> >> That way will have global 'debug' flag, but users will still have an
> >> ability to enable/disable debug flags on a per lib basis via
> >> CFLAGS="-D..."
> >>
> >> Konstantin
> >>
> > That seems a reasonable idea to me.
> >
> > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > either:
> >
> > * allow each component meson.build file define its own flags after
> > checking get_option('debug') * have lib/meson.build and
> > drivers/meson.build automatically define a specific define for each
> > library or driver to standardize the naming. [This would save anyone
> > working on it from having to lookup what the define was, since it's
> > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > RTE_DEBUG_SCHED etc]
> >
> > Theoretically we can also do both, have the standard ones defined and
> > then allow a component to provide extra flags itself if so desired.
> >
> > /Bruce
> OK, so let's summarize how the patches should be redo: * usage of global
> "debug" flag for meson build stays * we standardize names of debug flags
> in the components to RTE_DEBUG_ + components name * debug flag enables al
> the RTE_DEBUG_... flags
>
> This allow to easily use both: * the debug flag - to enable all debugs *
> or define manually RTE_DEBUG+component name, just for debug from a single
> component
>
> I like Bruce's idea of adding it to the lib/meson.build and
> drivers/meson.build. This way they will be added to dpdk_conf meson
> object and written then later to rte_build_config.h before compilation
> stage. All the other modules will be able to use these flags.
>
Sounds good to me (obviously!), but I'd like other feedback to ensure
others are ok with this before you spend too much effort implementing it.
For the drivers, the flag probably needs to include the category as well as
the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
name confusion. Those flags can then be checked inside individual
meson.build files to enable other debug flags if necessary e.g. in ixgbe,
you could theoretically do:
if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
...
endif
to enable more fine-grained control if so desired, and to maintain
compatibility with existing defines, again if so desired.
/Bruce
20/04/2020 19:11, Bruce Richardson:
> On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> >
> > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > >>>
> <snip>
> > >> I am agree with Cristian concern here: that patch removes ability to
> > >> enable/disable debug on particular library/PMD. If the purpose is to
> > >> minimize number of config compile options, I wonder can't it be done
> > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > >>
> > >> defines that are used by multiple libs, probably can be set in upper
> > >> layer meson.build.
> > >>
> > >> That way will have global 'debug' flag, but users will still have an
> > >> ability to enable/disable debug flags on a per lib basis via
> > >> CFLAGS="-D..."
> > >>
> > >> Konstantin
> > >>
> > > That seems a reasonable idea to me.
> > >
> > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > either:
> > >
> > > * allow each component meson.build file define its own flags after
> > > checking get_option('debug') * have lib/meson.build and
> > > drivers/meson.build automatically define a specific define for each
> > > library or driver to standardize the naming. [This would save anyone
> > > working on it from having to lookup what the define was, since it's
> > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > RTE_DEBUG_SCHED etc]
> > >
> > > Theoretically we can also do both, have the standard ones defined and
> > > then allow a component to provide extra flags itself if so desired.
> > >
> > > /Bruce
> > OK, so let's summarize how the patches should be redo: * usage of global
> > "debug" flag for meson build stays * we standardize names of debug flags
> > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > the RTE_DEBUG_... flags
> >
> > This allow to easily use both: * the debug flag - to enable all debugs *
> > or define manually RTE_DEBUG+component name, just for debug from a single
> > component
> >
> > I like Bruce's idea of adding it to the lib/meson.build and
> > drivers/meson.build. This way they will be added to dpdk_conf meson
> > object and written then later to rte_build_config.h before compilation
> > stage. All the other modules will be able to use these flags.
> >
> Sounds good to me (obviously!), but I'd like other feedback to ensure
> others are ok with this before you spend too much effort implementing it.
>
> For the drivers, the flag probably needs to include the category as well as
> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> name confusion. Those flags can then be checked inside individual
> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> you could theoretically do:
>
> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> ...
> endif
>
> to enable more fine-grained control if so desired, and to maintain
> compatibility with existing defines, again if so desired.
Nak the nak from Cristian.
We don't need all these flags.
If the user choose to compile DPDK for debug, every debug facilities
should be enabled. Then at runtime it is possible to enable/disable
the interesting logs.
If you need to disable something which is not a log,
you can align with the log level thanks to the function rte_log_can_log.
Please let's stop complicating things for the contributors and the users.
Note: I am strong on this position.
On Mon, Apr 20, 2020 at 06:11:30PM +0100, Bruce Richardson wrote:
> On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> >
> > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > >>>
> <snip>
> > >> I am agree with Cristian concern here: that patch removes ability to
> > >> enable/disable debug on particular library/PMD. If the purpose is to
> > >> minimize number of config compile options, I wonder can't it be done
> > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > >>
> > >> defines that are used by multiple libs, probably can be set in upper
> > >> layer meson.build.
> > >>
> > >> That way will have global 'debug' flag, but users will still have an
> > >> ability to enable/disable debug flags on a per lib basis via
> > >> CFLAGS="-D..."
> > >>
> > >> Konstantin
> > >>
> > > That seems a reasonable idea to me.
> > >
> > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > either:
> > >
> > > * allow each component meson.build file define its own flags after
> > > checking get_option('debug') * have lib/meson.build and
> > > drivers/meson.build automatically define a specific define for each
> > > library or driver to standardize the naming. [This would save anyone
> > > working on it from having to lookup what the define was, since it's
> > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > RTE_DEBUG_SCHED etc]
> > >
> > > Theoretically we can also do both, have the standard ones defined and
> > > then allow a component to provide extra flags itself if so desired.
> > >
> > > /Bruce
> > OK, so let's summarize how the patches should be redo: * usage of global
> > "debug" flag for meson build stays * we standardize names of debug flags
> > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > the RTE_DEBUG_... flags
> >
> > This allow to easily use both: * the debug flag - to enable all debugs *
> > or define manually RTE_DEBUG+component name, just for debug from a single
> > component
> >
> > I like Bruce's idea of adding it to the lib/meson.build and
> > drivers/meson.build. This way they will be added to dpdk_conf meson
> > object and written then later to rte_build_config.h before compilation
> > stage. All the other modules will be able to use these flags.
> >
> Sounds good to me (obviously!), but I'd like other feedback to ensure
> others are ok with this before you spend too much effort implementing it.
>
> For the drivers, the flag probably needs to include the category as well as
> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> name confusion. Those flags can then be checked inside individual
> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> you could theoretically do:
>
> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> ...
> endif
>
> to enable more fine-grained control if so desired, and to maintain
> compatibility with existing defines, again if so desired.
>
> /Bruce
Apologies for the self-reply, but I'd also like to ensure that basing it
all off the meson debug flag is the right thing to do. Currently that flag
is used to place debug info in the binary, which is quite different to
turning all a massive amount of prints from each component. Some drivers
etc. can be very chatty when debug flags are on.
A further option might be to have a general debug_drivers or
debug_libraries option, working with globs in the same was as the current
"disable_drivers" option does. That would allow more fine grain debugging
without resorting to manual CFLAG hacking, and it would also keep separate
the debug info from the compiler (which adds no perf hit) from debug prints
from drivers and libs which definitely do.
/Bruce
On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
> 20/04/2020 19:11, Bruce Richardson:
> > On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> > >
> > > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > > >>>
> > <snip>
> > > >> I am agree with Cristian concern here: that patch removes ability to
> > > >> enable/disable debug on particular library/PMD. If the purpose is to
> > > >> minimize number of config compile options, I wonder can't it be done
> > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > >>
> > > >> defines that are used by multiple libs, probably can be set in upper
> > > >> layer meson.build.
> > > >>
> > > >> That way will have global 'debug' flag, but users will still have an
> > > >> ability to enable/disable debug flags on a per lib basis via
> > > >> CFLAGS="-D..."
> > > >>
> > > >> Konstantin
> > > >>
> > > > That seems a reasonable idea to me.
> > > >
> > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > either:
> > > >
> > > > * allow each component meson.build file define its own flags after
> > > > checking get_option('debug') * have lib/meson.build and
> > > > drivers/meson.build automatically define a specific define for each
> > > > library or driver to standardize the naming. [This would save anyone
> > > > working on it from having to lookup what the define was, since it's
> > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > > RTE_DEBUG_SCHED etc]
> > > >
> > > > Theoretically we can also do both, have the standard ones defined and
> > > > then allow a component to provide extra flags itself if so desired.
> > > >
> > > > /Bruce
> > > OK, so let's summarize how the patches should be redo: * usage of global
> > > "debug" flag for meson build stays * we standardize names of debug flags
> > > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > the RTE_DEBUG_... flags
> > >
> > > This allow to easily use both: * the debug flag - to enable all debugs *
> > > or define manually RTE_DEBUG+component name, just for debug from a single
> > > component
> > >
> > > I like Bruce's idea of adding it to the lib/meson.build and
> > > drivers/meson.build. This way they will be added to dpdk_conf meson
> > > object and written then later to rte_build_config.h before compilation
> > > stage. All the other modules will be able to use these flags.
> > >
> > Sounds good to me (obviously!), but I'd like other feedback to ensure
> > others are ok with this before you spend too much effort implementing it.
> >
> > For the drivers, the flag probably needs to include the category as well as
> > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > name confusion. Those flags can then be checked inside individual
> > meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > you could theoretically do:
> >
> > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > ...
> > endif
> >
> > to enable more fine-grained control if so desired, and to maintain
> > compatibility with existing defines, again if so desired.
>
> Nak the nak from Cristian.
>
> We don't need all these flags.
> If the user choose to compile DPDK for debug, every debug facilities
> should be enabled. Then at runtime it is possible to enable/disable
> the interesting logs.
> If you need to disable something which is not a log,
> you can align with the log level thanks to the function rte_log_can_log.
>
> Please let's stop complicating things for the contributors and the users.
> Note: I am strong on this position.
>
Note, this means that you need to ensure all debug printouts from libs and
drivers are using the RTE_LOG macros so can be runtime controlled. I think
that may be some distance from reality right now.
Even if we do want all debug enabled from one flag, I'm still not 100%
convinced that the existing debug flag is the way to do, since it only adds
debug info to binary without affecting code generation.
W dniu 20.04.2020 o 19:30, Bruce Richardson pisze:
> On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
>> 20/04/2020 19:11, Bruce Richardson:
>>> On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
>>>> W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
>>>>> On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
>>> <snip>
>>>>>> I am agree with Cristian concern here: that patch removes ability to
>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
>>>>>> minimize number of config compile options, I wonder can't it be done
>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>>>>>
>>>>>> defines that are used by multiple libs, probably can be set in upper
>>>>>> layer meson.build.
>>>>>>
>>>>>> That way will have global 'debug' flag, but users will still have an
>>>>>> ability to enable/disable debug flags on a per lib basis via
>>>>>> CFLAGS="-D..."
>>>>>>
>>>>>> Konstantin
>>>>>>
>>>>> That seems a reasonable idea to me.
>>>>>
>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
>>>>> either:
>>>>>
>>>>> * allow each component meson.build file define its own flags after
>>>>> checking get_option('debug') * have lib/meson.build and
>>>>> drivers/meson.build automatically define a specific define for each
>>>>> library or driver to standardize the naming. [This would save anyone
>>>>> working on it from having to lookup what the define was, since it's
>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
>>>>> RTE_DEBUG_SCHED etc]
>>>>>
>>>>> Theoretically we can also do both, have the standard ones defined and
>>>>> then allow a component to provide extra flags itself if so desired.
>>>>>
>>>>> /Bruce
>>>> OK, so let's summarize how the patches should be redo: * usage of global
>>>> "debug" flag for meson build stays * we standardize names of debug flags
>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
>>>> the RTE_DEBUG_... flags
>>>>
>>>> This allow to easily use both: * the debug flag - to enable all debugs *
>>>> or define manually RTE_DEBUG+component name, just for debug from a single
>>>> component
>>>>
>>>> I like Bruce's idea of adding it to the lib/meson.build and
>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
>>>> object and written then later to rte_build_config.h before compilation
>>>> stage. All the other modules will be able to use these flags.
>>>>
>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
>>> others are ok with this before you spend too much effort implementing it.
>>>
>>> For the drivers, the flag probably needs to include the category as well as
>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
>>> name confusion. Those flags can then be checked inside individual
>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
>>> you could theoretically do:
>>>
>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
>>> ...
>>> endif
>>>
>>> to enable more fine-grained control if so desired, and to maintain
>>> compatibility with existing defines, again if so desired.
>> Nak the nak from Cristian.
>>
>> We don't need all these flags.
>> If the user choose to compile DPDK for debug, every debug facilities
>> should be enabled. Then at runtime it is possible to enable/disable
>> the interesting logs.
>> If you need to disable something which is not a log,
>> you can align with the log level thanks to the function rte_log_can_log.
>>
>> Please let's stop complicating things for the contributors and the users.
>> Note: I am strong on this position.
>>
> Note, this means that you need to ensure all debug printouts from libs and
> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> that may be some distance from reality right now.
>
> Even if we do want all debug enabled from one flag, I'm still not 100%
> convinced that the existing debug flag is the way to do, since it only adds
> debug info to binary without affecting code generation.
OK, I see that there are different opinions on what shape the debug flag
should look like.
So I think, I'll hold on work on any implementation until we all agree,
what do we want.
@Bruce: What code generation do you have on mind?
20/04/2020 19:30, Bruce Richardson:
> On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
> > 20/04/2020 19:11, Bruce Richardson:
> > > On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> > > > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > > > >>>
> > > <snip>
> > > > >> I am agree with Cristian concern here: that patch removes ability to
> > > > >> enable/disable debug on particular library/PMD. If the purpose is to
> > > > >> minimize number of config compile options, I wonder can't it be done
> > > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > > >>
> > > > >> defines that are used by multiple libs, probably can be set in upper
> > > > >> layer meson.build.
> > > > >>
> > > > >> That way will have global 'debug' flag, but users will still have an
> > > > >> ability to enable/disable debug flags on a per lib basis via
> > > > >> CFLAGS="-D..."
> > > > >>
> > > > >> Konstantin
> > > > >>
> > > > > That seems a reasonable idea to me.
> > > > >
> > > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > > either:
> > > > >
> > > > > * allow each component meson.build file define its own flags after
> > > > > checking get_option('debug') * have lib/meson.build and
> > > > > drivers/meson.build automatically define a specific define for each
> > > > > library or driver to standardize the naming. [This would save anyone
> > > > > working on it from having to lookup what the define was, since it's
> > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > > > RTE_DEBUG_SCHED etc]
> > > > >
> > > > > Theoretically we can also do both, have the standard ones defined and
> > > > > then allow a component to provide extra flags itself if so desired.
> > > > >
> > > > > /Bruce
> > > > OK, so let's summarize how the patches should be redo: * usage of global
> > > > "debug" flag for meson build stays * we standardize names of debug flags
> > > > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > > the RTE_DEBUG_... flags
> > > >
> > > > This allow to easily use both: * the debug flag - to enable all debugs *
> > > > or define manually RTE_DEBUG+component name, just for debug from a single
> > > > component
> > > >
> > > > I like Bruce's idea of adding it to the lib/meson.build and
> > > > drivers/meson.build. This way they will be added to dpdk_conf meson
> > > > object and written then later to rte_build_config.h before compilation
> > > > stage. All the other modules will be able to use these flags.
> > > >
> > > Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > others are ok with this before you spend too much effort implementing it.
> > >
> > > For the drivers, the flag probably needs to include the category as well as
> > > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > name confusion. Those flags can then be checked inside individual
> > > meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > you could theoretically do:
> > >
> > > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > ...
> > > endif
> > >
> > > to enable more fine-grained control if so desired, and to maintain
> > > compatibility with existing defines, again if so desired.
> >
> > Nak the nak from Cristian.
> >
> > We don't need all these flags.
> > If the user choose to compile DPDK for debug, every debug facilities
> > should be enabled. Then at runtime it is possible to enable/disable
> > the interesting logs.
> > If you need to disable something which is not a log,
> > you can align with the log level thanks to the function rte_log_can_log.
> >
> > Please let's stop complicating things for the contributors and the users.
> > Note: I am strong on this position.
> >
> Note, this means that you need to ensure all debug printouts from libs and
> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> that may be some distance from reality right now.
Perfect! Let's expose those nasty logs which are not (yet) controllable.
And next step is to block any patch in those drivers or libs,
until it is fixed. Dynamic logging should have been complete for long.
> Even if we do want all debug enabled from one flag, I'm still not 100%
> convinced that the existing debug flag is the way to do, since it only adds
> debug info to binary without affecting code generation.
OK, we want to keep this flag for gdb symbols only?
And add a new flag for debugging facilities which hurt the runtime performance?
On Mon, Apr 20, 2020 at 07:35:36PM +0200, Thomas Monjalon wrote:
> 20/04/2020 19:30, Bruce Richardson:
> > On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
> > > 20/04/2020 19:11, Bruce Richardson:
> > > > On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> > > > > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > > > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > > > > >>>
> > > > <snip>
> > > > > >> I am agree with Cristian concern here: that patch removes ability to
> > > > > >> enable/disable debug on particular library/PMD. If the purpose is to
> > > > > >> minimize number of config compile options, I wonder can't it be done
> > > > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > > > >>
> > > > > >> defines that are used by multiple libs, probably can be set in upper
> > > > > >> layer meson.build.
> > > > > >>
> > > > > >> That way will have global 'debug' flag, but users will still have an
> > > > > >> ability to enable/disable debug flags on a per lib basis via
> > > > > >> CFLAGS="-D..."
> > > > > >>
> > > > > >> Konstantin
> > > > > >>
> > > > > > That seems a reasonable idea to me.
> > > > > >
> > > > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > > > either:
> > > > > >
> > > > > > * allow each component meson.build file define its own flags after
> > > > > > checking get_option('debug') * have lib/meson.build and
> > > > > > drivers/meson.build automatically define a specific define for each
> > > > > > library or driver to standardize the naming. [This would save anyone
> > > > > > working on it from having to lookup what the define was, since it's
> > > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > > > > RTE_DEBUG_SCHED etc]
> > > > > >
> > > > > > Theoretically we can also do both, have the standard ones defined and
> > > > > > then allow a component to provide extra flags itself if so desired.
> > > > > >
> > > > > > /Bruce
> > > > > OK, so let's summarize how the patches should be redo: * usage of global
> > > > > "debug" flag for meson build stays * we standardize names of debug flags
> > > > > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > > > the RTE_DEBUG_... flags
> > > > >
> > > > > This allow to easily use both: * the debug flag - to enable all debugs *
> > > > > or define manually RTE_DEBUG+component name, just for debug from a single
> > > > > component
> > > > >
> > > > > I like Bruce's idea of adding it to the lib/meson.build and
> > > > > drivers/meson.build. This way they will be added to dpdk_conf meson
> > > > > object and written then later to rte_build_config.h before compilation
> > > > > stage. All the other modules will be able to use these flags.
> > > > >
> > > > Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > > others are ok with this before you spend too much effort implementing it.
> > > >
> > > > For the drivers, the flag probably needs to include the category as well as
> > > > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > > name confusion. Those flags can then be checked inside individual
> > > > meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > > you could theoretically do:
> > > >
> > > > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > > ...
> > > > endif
> > > >
> > > > to enable more fine-grained control if so desired, and to maintain
> > > > compatibility with existing defines, again if so desired.
> > >
> > > Nak the nak from Cristian.
> > >
> > > We don't need all these flags.
> > > If the user choose to compile DPDK for debug, every debug facilities
> > > should be enabled. Then at runtime it is possible to enable/disable
> > > the interesting logs.
> > > If you need to disable something which is not a log,
> > > you can align with the log level thanks to the function rte_log_can_log.
> > >
> > > Please let's stop complicating things for the contributors and the users.
> > > Note: I am strong on this position.
> > >
> > Note, this means that you need to ensure all debug printouts from libs and
> > drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > that may be some distance from reality right now.
>
> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> And next step is to block any patch in those drivers or libs,
> until it is fixed. Dynamic logging should have been complete for long.
>
I can live with that, I suppose. Do we have any idea of the magnitude of
the work required here?
> > Even if we do want all debug enabled from one flag, I'm still not 100%
> > convinced that the existing debug flag is the way to do, since it only adds
> > debug info to binary without affecting code generation.
>
> OK, we want to keep this flag for gdb symbols only?
> And add a new flag for debugging facilities which hurt the runtime performance?
>
I think that would be wise, yes. We can call the option "rte_debug" or
something instead.
/Bruce
> > > > > > >> I am agree with Cristian concern here: that patch removes ability to
> > > > > > >> enable/disable debug on particular library/PMD. If the purpose is to
> > > > > > >> minimize number of config compile options, I wonder can't it be done
> > > > > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > > > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > > > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > > > > >> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > > > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > > > > >>
> > > > > > >> defines that are used by multiple libs, probably can be set in upper
> > > > > > >> layer meson.build.
> > > > > > >>
> > > > > > >> That way will have global 'debug' flag, but users will still have an
> > > > > > >> ability to enable/disable debug flags on a per lib basis via
> > > > > > >> CFLAGS="-D..."
> > > > > > >>
> > > > > > >> Konstantin
> > > > > > >>
> > > > > > > That seems a reasonable idea to me.
> > > > > > >
> > > > > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > > > > either:
> > > > > > >
> > > > > > > * allow each component meson.build file define its own flags after
> > > > > > > checking get_option('debug') * have lib/meson.build and
> > > > > > > drivers/meson.build automatically define a specific define for each
> > > > > > > library or driver to standardize the naming. [This would save anyone
> > > > > > > working on it from having to lookup what the define was, since it's
> > > > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > > > > > RTE_DEBUG_SCHED etc]
> > > > > > >
> > > > > > > Theoretically we can also do both, have the standard ones defined and
> > > > > > > then allow a component to provide extra flags itself if so desired.
> > > > > > >
> > > > > > > /Bruce
> > > > > > OK, so let's summarize how the patches should be redo: * usage of global
> > > > > > "debug" flag for meson build stays * we standardize names of debug flags
> > > > > > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > > > > the RTE_DEBUG_... flags
> > > > > >
> > > > > > This allow to easily use both: * the debug flag - to enable all debugs *
> > > > > > or define manually RTE_DEBUG+component name, just for debug from a single
> > > > > > component
> > > > > >
> > > > > > I like Bruce's idea of adding it to the lib/meson.build and
> > > > > > drivers/meson.build. This way they will be added to dpdk_conf meson
> > > > > > object and written then later to rte_build_config.h before compilation
> > > > > > stage. All the other modules will be able to use these flags.
> > > > > >
> > > > > Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > > > others are ok with this before you spend too much effort implementing it.
> > > > >
> > > > > For the drivers, the flag probably needs to include the category as well as
> > > > > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > > > name confusion. Those flags can then be checked inside individual
> > > > > meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > > > you could theoretically do:
> > > > >
> > > > > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > > > cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > > > ...
> > > > > endif
> > > > >
> > > > > to enable more fine-grained control if so desired, and to maintain
> > > > > compatibility with existing defines, again if so desired.
> > > >
> > > > Nak the nak from Cristian.
> > > >
> > > > We don't need all these flags.
> > > > If the user choose to compile DPDK for debug, every debug facilities
> > > > should be enabled. Then at runtime it is possible to enable/disable
> > > > the interesting logs.
> > > > If you need to disable something which is not a log,
> > > > you can align with the log level thanks to the function rte_log_can_log.
For many libs these flags mean much more than just logging.
Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
drivers - extra validation performed.
RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
rte_mbuf_sanity_check() instead of just NOP.
Which means performance would be greatly affected.
RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
and enforce extra checking, stats collection.
etc.
Probably that's ok for some cases to enable all that extra validation we have at once.
But I suppose in many cases people just interested to enable debug on one
(ok might be two/three) particular libraries, not the whole system.
Right now there is such ability, we are going to remove it without
providing adequate replacement.
Approach with rte_log_can_log() seems workable,
but right now these patches don't implement it.
Konstantin
> > > >
> > > > Please let's stop complicating things for the contributors and the users.
> > > > Note: I am strong on this position.
> > > >
> > > Note, this means that you need to ensure all debug printouts from libs and
> > > drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > > that may be some distance from reality right now.
> >
> > Perfect! Let's expose those nasty logs which are not (yet) controllable.
> > And next step is to block any patch in those drivers or libs,
> > until it is fixed. Dynamic logging should have been complete for long.
> >
> I can live with that, I suppose. Do we have any idea of the magnitude of
> the work required here?
>
> > > Even if we do want all debug enabled from one flag, I'm still not 100%
> > > convinced that the existing debug flag is the way to do, since it only adds
> > > debug info to binary without affecting code generation.
> >
> > OK, we want to keep this flag for gdb symbols only?
> > And add a new flag for debugging facilities which hurt the runtime performance?
> >
> I think that would be wise, yes. We can call the option "rte_debug" or
> something instead.
>
> /Bruce
W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
>
>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
>>>>>>>>> minimize number of config compile options, I wonder can't it be done
>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>>>>>>>>
>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
>>>>>>>>> layer meson.build.
>>>>>>>>>
>>>>>>>>> That way will have global 'debug' flag, but users will still have an
>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
>>>>>>>>> CFLAGS="-D..."
>>>>>>>>>
>>>>>>>>> Konstantin
>>>>>>>>>
>>>>>>>> That seems a reasonable idea to me.
>>>>>>>>
>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
>>>>>>>> either:
>>>>>>>>
>>>>>>>> * allow each component meson.build file define its own flags after
>>>>>>>> checking get_option('debug') * have lib/meson.build and
>>>>>>>> drivers/meson.build automatically define a specific define for each
>>>>>>>> library or driver to standardize the naming. [This would save anyone
>>>>>>>> working on it from having to lookup what the define was, since it's
>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
>>>>>>>> RTE_DEBUG_SCHED etc]
>>>>>>>>
>>>>>>>> Theoretically we can also do both, have the standard ones defined and
>>>>>>>> then allow a component to provide extra flags itself if so desired.
>>>>>>>>
>>>>>>>> /Bruce
>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
>>>>>>> the RTE_DEBUG_... flags
>>>>>>>
>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
>>>>>>> component
>>>>>>>
>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
>>>>>>> object and written then later to rte_build_config.h before compilation
>>>>>>> stage. All the other modules will be able to use these flags.
>>>>>>>
>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
>>>>>> others are ok with this before you spend too much effort implementing it.
>>>>>>
>>>>>> For the drivers, the flag probably needs to include the category as well as
>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
>>>>>> name confusion. Those flags can then be checked inside individual
>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
>>>>>> you could theoretically do:
>>>>>>
>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
>>>>>> ...
>>>>>> endif
>>>>>>
>>>>>> to enable more fine-grained control if so desired, and to maintain
>>>>>> compatibility with existing defines, again if so desired.
>>>>> Nak the nak from Cristian.
>>>>>
>>>>> We don't need all these flags.
>>>>> If the user choose to compile DPDK for debug, every debug facilities
>>>>> should be enabled. Then at runtime it is possible to enable/disable
>>>>> the interesting logs.
>>>>> If you need to disable something which is not a log,
>>>>> you can align with the log level thanks to the function rte_log_can_log.
> For many libs these flags mean much more than just logging.
> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> drivers - extra validation performed.
> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> rte_mbuf_sanity_check() instead of just NOP.
> Which means performance would be greatly affected.
> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> and enforce extra checking, stats collection.
> etc.
> Probably that's ok for some cases to enable all that extra validation we have at once.
> But I suppose in many cases people just interested to enable debug on one
> (ok might be two/three) particular libraries, not the whole system.
> Right now there is such ability, we are going to remove it without
> providing adequate replacement.
> Approach with rte_log_can_log() seems workable,
> but right now these patches don't implement it.
> Konstantin
>
>>>>> Please let's stop complicating things for the contributors and the users.
>>>>> Note: I am strong on this position.
>>>>>
>>>> Note, this means that you need to ensure all debug printouts from libs and
>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
>>>> that may be some distance from reality right now.
>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
>>> And next step is to block any patch in those drivers or libs,
>>> until it is fixed. Dynamic logging should have been complete for long.
>>>
>> I can live with that, I suppose. Do we have any idea of the magnitude of
>> the work required here?
>>
>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
>>>> convinced that the existing debug flag is the way to do, since it only adds
>>>> debug info to binary without affecting code generation.
>>> OK, we want to keep this flag for gdb symbols only?
>>> And add a new flag for debugging facilities which hurt the runtime performance?
>>>
>> I think that would be wise, yes. We can call the option "rte_debug" or
>> something instead.
>>
>> /Bruce
OK, lets's summarize current opinions and requirements to make a
proposal for version2 of these patches, that I can implement if all agree:
1) The global debug flag is required to enable all the sanity checks and
validations that are normally not used due to performance reasons
2) The best option would be to have a single flag - not to introduce too
many build options
3) This option should be separated from meson "debug" option (used for
build with symbols) and can be called "rte_debug"
4) The currently existing DEBUG macros should not be replaced with a
RTE_DEBUG macro. This would allow to still enable them using
CFLAGS="-D..." to test a single module (library, driver).
5) Currently existing options' names should be standardized to
RTE_DEBUG_{library/driver name}, so they can be automatically enabled
when rte_debug is set. Standardized names would allow easy usage in
other modules.
Should they? Or should we leave the current debug macros? Please share
your opinions as I see both cons and pros of this idea.
6) To avoid logs flood using this option, logging functionality in the
debug section of the code should use valid logtype, so they can be
filtered out by rte_log_can_log().
This can be done in 2nd stage, when all logs not using proper logtype
will be exposed in rte_debug mode.
7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc.
in case of rt_debug all can be enabled in driver's internal meson.build
(as Bruce proposed above)
There is also an alternative proposed: defining options like
debug_drivers or debug_libraries that would use globs and work similar
way to the disable_drivers.
Please share you opinion about this plan!
21/04/2020 22:58, Lukasz Wojciechowski:
>
> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> >
> >>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> >>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> >>>>>>>>> minimize number of config compile options, I wonder can't it be done
> >>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> >>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> >>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> >>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> >>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> >>>>>>>>>
> >>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> >>>>>>>>> layer meson.build.
> >>>>>>>>>
> >>>>>>>>> That way will have global 'debug' flag, but users will still have an
> >>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> >>>>>>>>> CFLAGS="-D..."
> >>>>>>>>>
> >>>>>>>>> Konstantin
> >>>>>>>>>
> >>>>>>>> That seems a reasonable idea to me.
> >>>>>>>>
> >>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> >>>>>>>> either:
> >>>>>>>>
> >>>>>>>> * allow each component meson.build file define its own flags after
> >>>>>>>> checking get_option('debug') * have lib/meson.build and
> >>>>>>>> drivers/meson.build automatically define a specific define for each
> >>>>>>>> library or driver to standardize the naming. [This would save anyone
> >>>>>>>> working on it from having to lookup what the define was, since it's
> >>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> >>>>>>>> RTE_DEBUG_SCHED etc]
> >>>>>>>>
> >>>>>>>> Theoretically we can also do both, have the standard ones defined and
> >>>>>>>> then allow a component to provide extra flags itself if so desired.
> >>>>>>>>
> >>>>>>>> /Bruce
> >>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> >>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> >>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> >>>>>>> the RTE_DEBUG_... flags
> >>>>>>>
> >>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> >>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> >>>>>>> component
> >>>>>>>
> >>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> >>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> >>>>>>> object and written then later to rte_build_config.h before compilation
> >>>>>>> stage. All the other modules will be able to use these flags.
> >>>>>>>
> >>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> >>>>>> others are ok with this before you spend too much effort implementing it.
> >>>>>>
> >>>>>> For the drivers, the flag probably needs to include the category as well as
> >>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> >>>>>> name confusion. Those flags can then be checked inside individual
> >>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> >>>>>> you could theoretically do:
> >>>>>>
> >>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> >>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> >>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> >>>>>> ...
> >>>>>> endif
> >>>>>>
> >>>>>> to enable more fine-grained control if so desired, and to maintain
> >>>>>> compatibility with existing defines, again if so desired.
> >>>>> Nak the nak from Cristian.
> >>>>>
> >>>>> We don't need all these flags.
> >>>>> If the user choose to compile DPDK for debug, every debug facilities
> >>>>> should be enabled. Then at runtime it is possible to enable/disable
> >>>>> the interesting logs.
> >>>>> If you need to disable something which is not a log,
> >>>>> you can align with the log level thanks to the function rte_log_can_log.
> > For many libs these flags mean much more than just logging.
> > Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > drivers - extra validation performed.
> > RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> > rte_mbuf_sanity_check() instead of just NOP.
> > Which means performance would be greatly affected.
> > RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > and enforce extra checking, stats collection.
> > etc.
> > Probably that's ok for some cases to enable all that extra validation we have at once.
> > But I suppose in many cases people just interested to enable debug on one
> > (ok might be two/three) particular libraries, not the whole system.
> > Right now there is such ability, we are going to remove it without
> > providing adequate replacement.
> > Approach with rte_log_can_log() seems workable,
> > but right now these patches don't implement it.
> > Konstantin
> >
> >>>>> Please let's stop complicating things for the contributors and the users.
> >>>>> Note: I am strong on this position.
> >>>>>
> >>>> Note, this means that you need to ensure all debug printouts from libs and
> >>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> >>>> that may be some distance from reality right now.
> >>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> >>> And next step is to block any patch in those drivers or libs,
> >>> until it is fixed. Dynamic logging should have been complete for long.
> >>>
> >> I can live with that, I suppose. Do we have any idea of the magnitude of
> >> the work required here?
> >>
> >>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> >>>> convinced that the existing debug flag is the way to do, since it only adds
> >>>> debug info to binary without affecting code generation.
> >>> OK, we want to keep this flag for gdb symbols only?
> >>> And add a new flag for debugging facilities which hurt the runtime performance?
> >>>
> >> I think that would be wise, yes. We can call the option "rte_debug" or
> >> something instead.
> >>
> >> /Bruce
> OK, lets's summarize current opinions and requirements to make a
> proposal for version2 of these patches, that I can implement if all agree:
>
> 1) The global debug flag is required to enable all the sanity checks and
> validations that are normally not used due to performance reasons
Yes
> 2) The best option would be to have a single flag - not to introduce too
> many build options
Yes
> 3) This option should be separated from meson "debug" option (used for
> build with symbols) and can be called "rte_debug"
Yes, it looks to be the consensus.
> 4) The currently existing DEBUG macros should not be replaced with a
> RTE_DEBUG macro. This would allow to still enable them using
> CFLAGS="-D..." to test a single module (library, driver).
>
> 5) Currently existing options' names should be standardized to
> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> when rte_debug is set. Standardized names would allow easy usage in
> other modules.
I don't understand difference between 4) et 5).
> Should they? Or should we leave the current debug macros? Please share
> your opinions as I see both cons and pros of this idea.
I am not sure we need to keep fine-grain debug flags per libs/drivers.
In case RTE_DEBUG is enabled, which kind of debug processing
(except logs) do we want to be able to disable?
Is it possible to decide based on a call to rte_log_can_log()?
> 6) To avoid logs flood using this option, logging functionality in the
> debug section of the code should use valid logtype, so they can be
> filtered out by rte_log_can_log().
rte_log_can_log() is not for logs. It allows to enable/disable some
code following the same rule as a logtype.
> This can be done in 2nd stage, when all logs not using proper logtype
> will be exposed in rte_debug mode.
>
> 7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc.
> in case of rt_debug all can be enabled in driver's internal meson.build
> (as Bruce proposed above)
>
>
> There is also an alternative proposed: defining options like
> debug_drivers or debug_libraries that would use globs and work similar
> way to the disable_drivers.
>
>
> Please share you opinion about this plan!
W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> 21/04/2020 22:58, Lukasz Wojciechowski:
>> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
>>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
>>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
>>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
>>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
>>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
>>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
>>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
>>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>>>>>>>>>>
>>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
>>>>>>>>>>> layer meson.build.
>>>>>>>>>>>
>>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
>>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
>>>>>>>>>>> CFLAGS="-D..."
>>>>>>>>>>>
>>>>>>>>>>> Konstantin
>>>>>>>>>>>
>>>>>>>>>> That seems a reasonable idea to me.
>>>>>>>>>>
>>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
>>>>>>>>>> either:
>>>>>>>>>>
>>>>>>>>>> * allow each component meson.build file define its own flags after
>>>>>>>>>> checking get_option('debug') * have lib/meson.build and
>>>>>>>>>> drivers/meson.build automatically define a specific define for each
>>>>>>>>>> library or driver to standardize the naming. [This would save anyone
>>>>>>>>>> working on it from having to lookup what the define was, since it's
>>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
>>>>>>>>>> RTE_DEBUG_SCHED etc]
>>>>>>>>>>
>>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
>>>>>>>>>> then allow a component to provide extra flags itself if so desired.
>>>>>>>>>>
>>>>>>>>>> /Bruce
>>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
>>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
>>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
>>>>>>>>> the RTE_DEBUG_... flags
>>>>>>>>>
>>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
>>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
>>>>>>>>> component
>>>>>>>>>
>>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
>>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
>>>>>>>>> object and written then later to rte_build_config.h before compilation
>>>>>>>>> stage. All the other modules will be able to use these flags.
>>>>>>>>>
>>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
>>>>>>>> others are ok with this before you spend too much effort implementing it.
>>>>>>>>
>>>>>>>> For the drivers, the flag probably needs to include the category as well as
>>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
>>>>>>>> name confusion. Those flags can then be checked inside individual
>>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
>>>>>>>> you could theoretically do:
>>>>>>>>
>>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
>>>>>>>> ...
>>>>>>>> endif
>>>>>>>>
>>>>>>>> to enable more fine-grained control if so desired, and to maintain
>>>>>>>> compatibility with existing defines, again if so desired.
>>>>>>> Nak the nak from Cristian.
>>>>>>>
>>>>>>> We don't need all these flags.
>>>>>>> If the user choose to compile DPDK for debug, every debug facilities
>>>>>>> should be enabled. Then at runtime it is possible to enable/disable
>>>>>>> the interesting logs.
>>>>>>> If you need to disable something which is not a log,
>>>>>>> you can align with the log level thanks to the function rte_log_can_log.
>>> For many libs these flags mean much more than just logging.
>>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
>>> drivers - extra validation performed.
>>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
>>> rte_mbuf_sanity_check() instead of just NOP.
>>> Which means performance would be greatly affected.
>>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
>>> and enforce extra checking, stats collection.
>>> etc.
>>> Probably that's ok for some cases to enable all that extra validation we have at once.
>>> But I suppose in many cases people just interested to enable debug on one
>>> (ok might be two/three) particular libraries, not the whole system.
>>> Right now there is such ability, we are going to remove it without
>>> providing adequate replacement.
>>> Approach with rte_log_can_log() seems workable,
>>> but right now these patches don't implement it.
>>> Konstantin
>>>
>>>>>>> Please let's stop complicating things for the contributors and the users.
>>>>>>> Note: I am strong on this position.
>>>>>>>
>>>>>> Note, this means that you need to ensure all debug printouts from libs and
>>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
>>>>>> that may be some distance from reality right now.
>>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
>>>>> And next step is to block any patch in those drivers or libs,
>>>>> until it is fixed. Dynamic logging should have been complete for long.
>>>>>
>>>> I can live with that, I suppose. Do we have any idea of the magnitude of
>>>> the work required here?
>>>>
>>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
>>>>>> convinced that the existing debug flag is the way to do, since it only adds
>>>>>> debug info to binary without affecting code generation.
>>>>> OK, we want to keep this flag for gdb symbols only?
>>>>> And add a new flag for debugging facilities which hurt the runtime performance?
>>>>>
>>>> I think that would be wise, yes. We can call the option "rte_debug" or
>>>> something instead.
>>>>
>>>> /Bruce
>> OK, lets's summarize current opinions and requirements to make a
>> proposal for version2 of these patches, that I can implement if all agree:
>>
>> 1) The global debug flag is required to enable all the sanity checks and
>> validations that are normally not used due to performance reasons
> Yes
>
>> 2) The best option would be to have a single flag - not to introduce too
>> many build options
> Yes
>
>> 3) This option should be separated from meson "debug" option (used for
>> build with symbols) and can be called "rte_debug"
> Yes, it looks to be the consensus.
>
>> 4) The currently existing DEBUG macros should not be replaced with a
>> RTE_DEBUG macro. This would allow to still enable them using
>> CFLAGS="-D..." to test a single module (library, driver).
>>
>> 5) Currently existing options' names should be standardized to
>> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
>> when rte_debug is set. Standardized names would allow easy usage in
>> other modules.
> I don't understand difference between 4) et 5).
In current version of patches, I replaced all the DEBUG macros with
RTE_DEBUG. It would be much better to keep fine-grained debugs as they
are defined currently in dpdk. This is what I have on mind in 4)
However the currently used debug macros have different naming
conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
RTE_{name}_DEBUG, some just {name}_DEBUG.
So in 5) I follow Bruce's proposal to standardize them to one form
RTE_DEBUG_{name}. However this will change the existing macros and
someone might not like it, so I ask for the opinion about it.
>> Should they? Or should we leave the current debug macros? Please share
>> your opinions as I see both cons and pros of this idea.
> I am not sure we need to keep fine-grain debug flags per libs/drivers.
> In case RTE_DEBUG is enabled, which kind of debug processing
> (except logs) do we want to be able to disable?
> Is it possible to decide based on a call to rte_log_can_log()?
I think it's rather opposite way round. Sometimes we would like to
enable just some debug processing, e.g. when working on single lib or
driver.
If we will use rte_debug - every debug processing would be enabled, we
won't disable anything, but without rte_debug we will still have the
possibility of enabling debugs on a single module.
I believe it is possible to do it with rte_log_can_log, but changing
build time enabled options into runtime enabled options might affect
performance. It might make working on a single library or driver harder.
>
>> 6) To avoid logs flood using this option, logging functionality in the
>> debug section of the code should use valid logtype, so they can be
>> filtered out by rte_log_can_log().
> rte_log_can_log() is not for logs. It allows to enable/disable some
> code following the same rule as a logtype.
Oh, I didn't know, that it should be used this way also.
Currently except for logs it's used in 5 places and are related with
printing some information. So I thought it was just for logging.
>
>> This can be done in 2nd stage, when all logs not using proper logtype
>> will be exposed in rte_debug mode.
>>
>> 7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc.
>> in case of rt_debug all can be enabled in driver's internal meson.build
>> (as Bruce proposed above)
>>
>>
>> There is also an alternative proposed: defining options like
>> debug_drivers or debug_libraries that would use globs and work similar
>> way to the disable_drivers.
>>
>>
>> Please share you opinion about this plan!
>
>
>
On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
>
> W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > 21/04/2020 22:58, Lukasz Wojciechowski:
> >> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> >>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> >>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> >>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> >>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> >>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> >>>>>>>>>>>
> >>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> >>>>>>>>>>> layer meson.build.
> >>>>>>>>>>>
> >>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> >>>>>>>>>>> CFLAGS="-D..."
> >>>>>>>>>>>
> >>>>>>>>>>> Konstantin
> >>>>>>>>>>>
> >>>>>>>>>> That seems a reasonable idea to me.
> >>>>>>>>>>
> >>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> >>>>>>>>>> either:
> >>>>>>>>>>
> >>>>>>>>>> * allow each component meson.build file define its own flags after
> >>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> >>>>>>>>>> drivers/meson.build automatically define a specific define for each
> >>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> >>>>>>>>>> working on it from having to lookup what the define was, since it's
> >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> >>>>>>>>>> RTE_DEBUG_SCHED etc]
> >>>>>>>>>>
> >>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> >>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> >>>>>>>>>>
> >>>>>>>>>> /Bruce
> >>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> >>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> >>>>>>>>> the RTE_DEBUG_... flags
> >>>>>>>>>
> >>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> >>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> >>>>>>>>> component
> >>>>>>>>>
> >>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> >>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> >>>>>>>>> object and written then later to rte_build_config.h before compilation
> >>>>>>>>> stage. All the other modules will be able to use these flags.
> >>>>>>>>>
> >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> >>>>>>>> others are ok with this before you spend too much effort implementing it.
> >>>>>>>>
> >>>>>>>> For the drivers, the flag probably needs to include the category as well as
> >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> >>>>>>>> name confusion. Those flags can then be checked inside individual
> >>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> >>>>>>>> you could theoretically do:
> >>>>>>>>
> >>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> >>>>>>>> ...
> >>>>>>>> endif
> >>>>>>>>
> >>>>>>>> to enable more fine-grained control if so desired, and to maintain
> >>>>>>>> compatibility with existing defines, again if so desired.
> >>>>>>> Nak the nak from Cristian.
> >>>>>>>
> >>>>>>> We don't need all these flags.
> >>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> >>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> >>>>>>> the interesting logs.
> >>>>>>> If you need to disable something which is not a log,
> >>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> >>> For many libs these flags mean much more than just logging.
> >>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> >>> drivers - extra validation performed.
> >>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> >>> rte_mbuf_sanity_check() instead of just NOP.
> >>> Which means performance would be greatly affected.
> >>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> >>> and enforce extra checking, stats collection.
> >>> etc.
> >>> Probably that's ok for some cases to enable all that extra validation we have at once.
> >>> But I suppose in many cases people just interested to enable debug on one
> >>> (ok might be two/three) particular libraries, not the whole system.
> >>> Right now there is such ability, we are going to remove it without
> >>> providing adequate replacement.
> >>> Approach with rte_log_can_log() seems workable,
> >>> but right now these patches don't implement it.
> >>> Konstantin
> >>>
> >>>>>>> Please let's stop complicating things for the contributors and the users.
> >>>>>>> Note: I am strong on this position.
> >>>>>>>
> >>>>>> Note, this means that you need to ensure all debug printouts from libs and
> >>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> >>>>>> that may be some distance from reality right now.
> >>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> >>>>> And next step is to block any patch in those drivers or libs,
> >>>>> until it is fixed. Dynamic logging should have been complete for long.
> >>>>>
> >>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> >>>> the work required here?
> >>>>
> >>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> >>>>>> convinced that the existing debug flag is the way to do, since it only adds
> >>>>>> debug info to binary without affecting code generation.
> >>>>> OK, we want to keep this flag for gdb symbols only?
> >>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> >>>>>
> >>>> I think that would be wise, yes. We can call the option "rte_debug" or
> >>>> something instead.
> >>>>
> >>>> /Bruce
> >> OK, lets's summarize current opinions and requirements to make a
> >> proposal for version2 of these patches, that I can implement if all agree:
> >>
> >> 1) The global debug flag is required to enable all the sanity checks and
> >> validations that are normally not used due to performance reasons
> > Yes
> >
> >> 2) The best option would be to have a single flag - not to introduce too
> >> many build options
> > Yes
> >
> >> 3) This option should be separated from meson "debug" option (used for
> >> build with symbols) and can be called "rte_debug"
> > Yes, it looks to be the consensus.
> >
> >> 4) The currently existing DEBUG macros should not be replaced with a
> >> RTE_DEBUG macro. This would allow to still enable them using
> >> CFLAGS="-D..." to test a single module (library, driver).
> >>
> >> 5) Currently existing options' names should be standardized to
> >> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> >> when rte_debug is set. Standardized names would allow easy usage in
> >> other modules.
> > I don't understand difference between 4) et 5).
>
> In current version of patches, I replaced all the DEBUG macros with
> RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> are defined currently in dpdk. This is what I have on mind in 4)
>
> However the currently used debug macros have different naming
> conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> RTE_{name}_DEBUG, some just {name}_DEBUG.
> So in 5) I follow Bruce's proposal to standardize them to one form
> RTE_DEBUG_{name}. However this will change the existing macros and
> someone might not like it, so I ask for the opinion about it.
>
My thought is to standardize in the build and then leave it to module
owners to either change their macros to use those standard ones as time
goes on.
>
> >> Should they? Or should we leave the current debug macros? Please share
> >> your opinions as I see both cons and pros of this idea.
> > I am not sure we need to keep fine-grain debug flags per libs/drivers.
> > In case RTE_DEBUG is enabled, which kind of debug processing
> > (except logs) do we want to be able to disable?
> > Is it possible to decide based on a call to rte_log_can_log()?
> I think it's rather opposite way round. Sometimes we would like to
> enable just some debug processing, e.g. when working on single lib or
> driver.
> If we will use rte_debug - every debug processing would be enabled, we
> won't disable anything, but without rte_debug we will still have the
> possibility of enabling debugs on a single module.
>
> I believe it is possible to do it with rte_log_can_log, but changing
> build time enabled options into runtime enabled options might affect
> performance. It might make working on a single library or driver harder.
>
I think the idea is to use both. When RTE_DEBUG is enabled, then the
rte_log_can_log() calls will be used to control the actual output. Without
RTE_DEBUG, the whole block is skipped.
#ifdef RTE_DEBUG
if rte_log_can_log(...) {
/* do debug stuff */
}
#endif
22/04/2020 12:55, Bruce Richardson:
> On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> > W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > > 21/04/2020 22:58, Lukasz Wojciechowski:
> > >> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> > >>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> > >>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> > >>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> > >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > >>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > >>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > >>>>>>>>>>>
> > >>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> > >>>>>>>>>>> layer meson.build.
> > >>>>>>>>>>>
> > >>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> > >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> > >>>>>>>>>>> CFLAGS="-D..."
> > >>>>>>>>>>>
> > >>>>>>>>>>> Konstantin
> > >>>>>>>>>>>
> > >>>>>>>>>> That seems a reasonable idea to me.
> > >>>>>>>>>>
> > >>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > >>>>>>>>>> either:
> > >>>>>>>>>>
> > >>>>>>>>>> * allow each component meson.build file define its own flags after
> > >>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> > >>>>>>>>>> drivers/meson.build automatically define a specific define for each
> > >>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> > >>>>>>>>>> working on it from having to lookup what the define was, since it's
> > >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > >>>>>>>>>> RTE_DEBUG_SCHED etc]
> > >>>>>>>>>>
> > >>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> > >>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> > >>>>>>>>>>
> > >>>>>>>>>> /Bruce
> > >>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> > >>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> > >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> > >>>>>>>>> the RTE_DEBUG_... flags
> > >>>>>>>>>
> > >>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> > >>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> > >>>>>>>>> component
> > >>>>>>>>>
> > >>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> > >>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> > >>>>>>>>> object and written then later to rte_build_config.h before compilation
> > >>>>>>>>> stage. All the other modules will be able to use these flags.
> > >>>>>>>>>
> > >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> > >>>>>>>> others are ok with this before you spend too much effort implementing it.
> > >>>>>>>>
> > >>>>>>>> For the drivers, the flag probably needs to include the category as well as
> > >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > >>>>>>>> name confusion. Those flags can then be checked inside individual
> > >>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > >>>>>>>> you could theoretically do:
> > >>>>>>>>
> > >>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > >>>>>>>> ...
> > >>>>>>>> endif
> > >>>>>>>>
> > >>>>>>>> to enable more fine-grained control if so desired, and to maintain
> > >>>>>>>> compatibility with existing defines, again if so desired.
> > >>>>>>> Nak the nak from Cristian.
> > >>>>>>>
> > >>>>>>> We don't need all these flags.
> > >>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> > >>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> > >>>>>>> the interesting logs.
> > >>>>>>> If you need to disable something which is not a log,
> > >>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> > >>> For many libs these flags mean much more than just logging.
> > >>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > >>> drivers - extra validation performed.
> > >>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> > >>> rte_mbuf_sanity_check() instead of just NOP.
> > >>> Which means performance would be greatly affected.
> > >>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > >>> and enforce extra checking, stats collection.
> > >>> etc.
> > >>> Probably that's ok for some cases to enable all that extra validation we have at once.
> > >>> But I suppose in many cases people just interested to enable debug on one
> > >>> (ok might be two/three) particular libraries, not the whole system.
> > >>> Right now there is such ability, we are going to remove it without
> > >>> providing adequate replacement.
> > >>> Approach with rte_log_can_log() seems workable,
> > >>> but right now these patches don't implement it.
> > >>> Konstantin
> > >>>
> > >>>>>>> Please let's stop complicating things for the contributors and the users.
> > >>>>>>> Note: I am strong on this position.
> > >>>>>>>
> > >>>>>> Note, this means that you need to ensure all debug printouts from libs and
> > >>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > >>>>>> that may be some distance from reality right now.
> > >>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> > >>>>> And next step is to block any patch in those drivers or libs,
> > >>>>> until it is fixed. Dynamic logging should have been complete for long.
> > >>>>>
> > >>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> > >>>> the work required here?
> > >>>>
> > >>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> > >>>>>> convinced that the existing debug flag is the way to do, since it only adds
> > >>>>>> debug info to binary without affecting code generation.
> > >>>>> OK, we want to keep this flag for gdb symbols only?
> > >>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> > >>>>>
> > >>>> I think that would be wise, yes. We can call the option "rte_debug" or
> > >>>> something instead.
> > >>>>
> > >>>> /Bruce
> > >> OK, lets's summarize current opinions and requirements to make a
> > >> proposal for version2 of these patches, that I can implement if all agree:
> > >>
> > >> 1) The global debug flag is required to enable all the sanity checks and
> > >> validations that are normally not used due to performance reasons
> > > Yes
> > >
> > >> 2) The best option would be to have a single flag - not to introduce too
> > >> many build options
> > > Yes
> > >
> > >> 3) This option should be separated from meson "debug" option (used for
> > >> build with symbols) and can be called "rte_debug"
> > > Yes, it looks to be the consensus.
> > >
> > >> 4) The currently existing DEBUG macros should not be replaced with a
> > >> RTE_DEBUG macro. This would allow to still enable them using
> > >> CFLAGS="-D..." to test a single module (library, driver).
> > >>
> > >> 5) Currently existing options' names should be standardized to
> > >> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> > >> when rte_debug is set. Standardized names would allow easy usage in
> > >> other modules.
> > > I don't understand difference between 4) et 5).
> >
> > In current version of patches, I replaced all the DEBUG macros with
> > RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> > are defined currently in dpdk. This is what I have on mind in 4)
> >
> > However the currently used debug macros have different naming
> > conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> > RTE_{name}_DEBUG, some just {name}_DEBUG.
> > So in 5) I follow Bruce's proposal to standardize them to one form
> > RTE_DEBUG_{name}. However this will change the existing macros and
> > someone might not like it, so I ask for the opinion about it.
> >
> My thought is to standardize in the build and then leave it to module
> owners to either change their macros to use those standard ones as time
> goes on.
In order to maintain a good global user experience,
we need to drive such change with a roadmap and deadlines.
> > >> Should they? Or should we leave the current debug macros? Please share
> > >> your opinions as I see both cons and pros of this idea.
> > > I am not sure we need to keep fine-grain debug flags per libs/drivers.
> > > In case RTE_DEBUG is enabled, which kind of debug processing
> > > (except logs) do we want to be able to disable?
> > > Is it possible to decide based on a call to rte_log_can_log()?
> > I think it's rather opposite way round. Sometimes we would like to
> > enable just some debug processing, e.g. when working on single lib or
> > driver.
> > If we will use rte_debug - every debug processing would be enabled, we
> > won't disable anything, but without rte_debug we will still have the
> > possibility of enabling debugs on a single module.
> >
> > I believe it is possible to do it with rte_log_can_log, but changing
> > build time enabled options into runtime enabled options might affect
> > performance. It might make working on a single library or driver harder.
> >
>
> I think the idea is to use both. When RTE_DEBUG is enabled, then the
> rte_log_can_log() calls will be used to control the actual output. Without
> RTE_DEBUG, the whole block is skipped.
>
> #ifdef RTE_DEBUG
> if rte_log_can_log(...) {
> /* do debug stuff */
> }
> #endif
This is what I had in mind.
The question is about performance impact in the case
we enable RTE_DEBUG at compilation time, and don't enable a
specific debug at runtime: is this check overhead acceptable?
if rte_log_can_log(...)
On Wed, Apr 22, 2020 at 01:02:42PM +0200, Thomas Monjalon wrote:
> 22/04/2020 12:55, Bruce Richardson:
> > On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> > > W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > > > 21/04/2020 22:58, Lukasz Wojciechowski:
> > > >> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> > > >>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> > > >>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> > > >>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> > > >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > >>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > >>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> > > >>>>>>>>>>> layer meson.build.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> > > >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> > > >>>>>>>>>>> CFLAGS="-D..."
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Konstantin
> > > >>>>>>>>>>>
> > > >>>>>>>>>> That seems a reasonable idea to me.
> > > >>>>>>>>>>
> > > >>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > >>>>>>>>>> either:
> > > >>>>>>>>>>
> > > >>>>>>>>>> * allow each component meson.build file define its own flags after
> > > >>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> > > >>>>>>>>>> drivers/meson.build automatically define a specific define for each
> > > >>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> > > >>>>>>>>>> working on it from having to lookup what the define was, since it's
> > > >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > >>>>>>>>>> RTE_DEBUG_SCHED etc]
> > > >>>>>>>>>>
> > > >>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> > > >>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> > > >>>>>>>>>>
> > > >>>>>>>>>> /Bruce
> > > >>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> > > >>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> > > >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > >>>>>>>>> the RTE_DEBUG_... flags
> > > >>>>>>>>>
> > > >>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> > > >>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> > > >>>>>>>>> component
> > > >>>>>>>>>
> > > >>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> > > >>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> > > >>>>>>>>> object and written then later to rte_build_config.h before compilation
> > > >>>>>>>>> stage. All the other modules will be able to use these flags.
> > > >>>>>>>>>
> > > >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > >>>>>>>> others are ok with this before you spend too much effort implementing it.
> > > >>>>>>>>
> > > >>>>>>>> For the drivers, the flag probably needs to include the category as well as
> > > >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > >>>>>>>> name confusion. Those flags can then be checked inside individual
> > > >>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > >>>>>>>> you could theoretically do:
> > > >>>>>>>>
> > > >>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > >>>>>>>> ...
> > > >>>>>>>> endif
> > > >>>>>>>>
> > > >>>>>>>> to enable more fine-grained control if so desired, and to maintain
> > > >>>>>>>> compatibility with existing defines, again if so desired.
> > > >>>>>>> Nak the nak from Cristian.
> > > >>>>>>>
> > > >>>>>>> We don't need all these flags.
> > > >>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> > > >>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> > > >>>>>>> the interesting logs.
> > > >>>>>>> If you need to disable something which is not a log,
> > > >>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> > > >>> For many libs these flags mean much more than just logging.
> > > >>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > > >>> drivers - extra validation performed.
> > > >>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> > > >>> rte_mbuf_sanity_check() instead of just NOP.
> > > >>> Which means performance would be greatly affected.
> > > >>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > > >>> and enforce extra checking, stats collection.
> > > >>> etc.
> > > >>> Probably that's ok for some cases to enable all that extra validation we have at once.
> > > >>> But I suppose in many cases people just interested to enable debug on one
> > > >>> (ok might be two/three) particular libraries, not the whole system.
> > > >>> Right now there is such ability, we are going to remove it without
> > > >>> providing adequate replacement.
> > > >>> Approach with rte_log_can_log() seems workable,
> > > >>> but right now these patches don't implement it.
> > > >>> Konstantin
> > > >>>
> > > >>>>>>> Please let's stop complicating things for the contributors and the users.
> > > >>>>>>> Note: I am strong on this position.
> > > >>>>>>>
> > > >>>>>> Note, this means that you need to ensure all debug printouts from libs and
> > > >>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > > >>>>>> that may be some distance from reality right now.
> > > >>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> > > >>>>> And next step is to block any patch in those drivers or libs,
> > > >>>>> until it is fixed. Dynamic logging should have been complete for long.
> > > >>>>>
> > > >>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> > > >>>> the work required here?
> > > >>>>
> > > >>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> > > >>>>>> convinced that the existing debug flag is the way to do, since it only adds
> > > >>>>>> debug info to binary without affecting code generation.
> > > >>>>> OK, we want to keep this flag for gdb symbols only?
> > > >>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> > > >>>>>
> > > >>>> I think that would be wise, yes. We can call the option "rte_debug" or
> > > >>>> something instead.
> > > >>>>
> > > >>>> /Bruce
> > > >> OK, lets's summarize current opinions and requirements to make a
> > > >> proposal for version2 of these patches, that I can implement if all agree:
> > > >>
> > > >> 1) The global debug flag is required to enable all the sanity checks and
> > > >> validations that are normally not used due to performance reasons
> > > > Yes
> > > >
> > > >> 2) The best option would be to have a single flag - not to introduce too
> > > >> many build options
> > > > Yes
> > > >
> > > >> 3) This option should be separated from meson "debug" option (used for
> > > >> build with symbols) and can be called "rte_debug"
> > > > Yes, it looks to be the consensus.
> > > >
> > > >> 4) The currently existing DEBUG macros should not be replaced with a
> > > >> RTE_DEBUG macro. This would allow to still enable them using
> > > >> CFLAGS="-D..." to test a single module (library, driver).
> > > >>
> > > >> 5) Currently existing options' names should be standardized to
> > > >> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> > > >> when rte_debug is set. Standardized names would allow easy usage in
> > > >> other modules.
> > > > I don't understand difference between 4) et 5).
> > >
> > > In current version of patches, I replaced all the DEBUG macros with
> > > RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> > > are defined currently in dpdk. This is what I have on mind in 4)
> > >
> > > However the currently used debug macros have different naming
> > > conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> > > RTE_{name}_DEBUG, some just {name}_DEBUG.
> > > So in 5) I follow Bruce's proposal to standardize them to one form
> > > RTE_DEBUG_{name}. However this will change the existing macros and
> > > someone might not like it, so I ask for the opinion about it.
> > >
> > My thought is to standardize in the build and then leave it to module
> > owners to either change their macros to use those standard ones as time
> > goes on.
>
> In order to maintain a good global user experience,
> we need to drive such change with a roadmap and deadlines.
>
> > > >> Should they? Or should we leave the current debug macros? Please share
> > > >> your opinions as I see both cons and pros of this idea.
> > > > I am not sure we need to keep fine-grain debug flags per libs/drivers.
> > > > In case RTE_DEBUG is enabled, which kind of debug processing
> > > > (except logs) do we want to be able to disable?
> > > > Is it possible to decide based on a call to rte_log_can_log()?
> > > I think it's rather opposite way round. Sometimes we would like to
> > > enable just some debug processing, e.g. when working on single lib or
> > > driver.
> > > If we will use rte_debug - every debug processing would be enabled, we
> > > won't disable anything, but without rte_debug we will still have the
> > > possibility of enabling debugs on a single module.
> > >
> > > I believe it is possible to do it with rte_log_can_log, but changing
> > > build time enabled options into runtime enabled options might affect
> > > performance. It might make working on a single library or driver harder.
> > >
> >
> > I think the idea is to use both. When RTE_DEBUG is enabled, then the
> > rte_log_can_log() calls will be used to control the actual output. Without
> > RTE_DEBUG, the whole block is skipped.
> >
> > #ifdef RTE_DEBUG
> > if rte_log_can_log(...) {
> > /* do debug stuff */
> > }
> > #endif
>
> This is what I had in mind.
> The question is about performance impact in the case
> we enable RTE_DEBUG at compilation time, and don't enable a
> specific debug at runtime: is this check overhead acceptable?
> if rte_log_can_log(...)
>
I believe it should be fine since:
* at runtime the branch will be predictable, since we don't go changing the
logging level each time through the code path.
* there is the ability to disable at build time, and it will be off by
default anyway, so only developers will use it.
* for non-developer investigation at runtime, the tracing library is the
preferred option.
/Bruce
>
> 22/04/2020 12:55, Bruce Richardson:
> > On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> > > W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > > > 21/04/2020 22:58, Lukasz Wojciechowski:
> > > >> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> > > >>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> > > >>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> > > >>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> > > >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > >>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > >>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> > > >>>>>>>>>>> layer meson.build.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> > > >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> > > >>>>>>>>>>> CFLAGS="-D..."
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Konstantin
> > > >>>>>>>>>>>
> > > >>>>>>>>>> That seems a reasonable idea to me.
> > > >>>>>>>>>>
> > > >>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > >>>>>>>>>> either:
> > > >>>>>>>>>>
> > > >>>>>>>>>> * allow each component meson.build file define its own flags after
> > > >>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> > > >>>>>>>>>> drivers/meson.build automatically define a specific define for each
> > > >>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> > > >>>>>>>>>> working on it from having to lookup what the define was, since it's
> > > >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > >>>>>>>>>> RTE_DEBUG_SCHED etc]
> > > >>>>>>>>>>
> > > >>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> > > >>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> > > >>>>>>>>>>
> > > >>>>>>>>>> /Bruce
> > > >>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> > > >>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> > > >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > >>>>>>>>> the RTE_DEBUG_... flags
> > > >>>>>>>>>
> > > >>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> > > >>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> > > >>>>>>>>> component
> > > >>>>>>>>>
> > > >>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> > > >>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> > > >>>>>>>>> object and written then later to rte_build_config.h before compilation
> > > >>>>>>>>> stage. All the other modules will be able to use these flags.
> > > >>>>>>>>>
> > > >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > >>>>>>>> others are ok with this before you spend too much effort implementing it.
> > > >>>>>>>>
> > > >>>>>>>> For the drivers, the flag probably needs to include the category as well as
> > > >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > >>>>>>>> name confusion. Those flags can then be checked inside individual
> > > >>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > >>>>>>>> you could theoretically do:
> > > >>>>>>>>
> > > >>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > >>>>>>>> ...
> > > >>>>>>>> endif
> > > >>>>>>>>
> > > >>>>>>>> to enable more fine-grained control if so desired, and to maintain
> > > >>>>>>>> compatibility with existing defines, again if so desired.
> > > >>>>>>> Nak the nak from Cristian.
> > > >>>>>>>
> > > >>>>>>> We don't need all these flags.
> > > >>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> > > >>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> > > >>>>>>> the interesting logs.
> > > >>>>>>> If you need to disable something which is not a log,
> > > >>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> > > >>> For many libs these flags mean much more than just logging.
> > > >>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > > >>> drivers - extra validation performed.
> > > >>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> > > >>> rte_mbuf_sanity_check() instead of just NOP.
> > > >>> Which means performance would be greatly affected.
> > > >>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > > >>> and enforce extra checking, stats collection.
> > > >>> etc.
> > > >>> Probably that's ok for some cases to enable all that extra validation we have at once.
> > > >>> But I suppose in many cases people just interested to enable debug on one
> > > >>> (ok might be two/three) particular libraries, not the whole system.
> > > >>> Right now there is such ability, we are going to remove it without
> > > >>> providing adequate replacement.
> > > >>> Approach with rte_log_can_log() seems workable,
> > > >>> but right now these patches don't implement it.
> > > >>> Konstantin
> > > >>>
> > > >>>>>>> Please let's stop complicating things for the contributors and the users.
> > > >>>>>>> Note: I am strong on this position.
> > > >>>>>>>
> > > >>>>>> Note, this means that you need to ensure all debug printouts from libs and
> > > >>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > > >>>>>> that may be some distance from reality right now.
> > > >>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> > > >>>>> And next step is to block any patch in those drivers or libs,
> > > >>>>> until it is fixed. Dynamic logging should have been complete for long.
> > > >>>>>
> > > >>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> > > >>>> the work required here?
> > > >>>>
> > > >>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> > > >>>>>> convinced that the existing debug flag is the way to do, since it only adds
> > > >>>>>> debug info to binary without affecting code generation.
> > > >>>>> OK, we want to keep this flag for gdb symbols only?
> > > >>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> > > >>>>>
> > > >>>> I think that would be wise, yes. We can call the option "rte_debug" or
> > > >>>> something instead.
> > > >>>>
> > > >>>> /Bruce
> > > >> OK, lets's summarize current opinions and requirements to make a
> > > >> proposal for version2 of these patches, that I can implement if all agree:
> > > >>
> > > >> 1) The global debug flag is required to enable all the sanity checks and
> > > >> validations that are normally not used due to performance reasons
> > > > Yes
> > > >
> > > >> 2) The best option would be to have a single flag - not to introduce too
> > > >> many build options
> > > > Yes
> > > >
> > > >> 3) This option should be separated from meson "debug" option (used for
> > > >> build with symbols) and can be called "rte_debug"
> > > > Yes, it looks to be the consensus.
> > > >
> > > >> 4) The currently existing DEBUG macros should not be replaced with a
> > > >> RTE_DEBUG macro. This would allow to still enable them using
> > > >> CFLAGS="-D..." to test a single module (library, driver).
> > > >>
> > > >> 5) Currently existing options' names should be standardized to
> > > >> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> > > >> when rte_debug is set. Standardized names would allow easy usage in
> > > >> other modules.
> > > > I don't understand difference between 4) et 5).
> > >
> > > In current version of patches, I replaced all the DEBUG macros with
> > > RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> > > are defined currently in dpdk. This is what I have on mind in 4)
> > >
> > > However the currently used debug macros have different naming
> > > conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> > > RTE_{name}_DEBUG, some just {name}_DEBUG.
> > > So in 5) I follow Bruce's proposal to standardize them to one form
> > > RTE_DEBUG_{name}. However this will change the existing macros and
> > > someone might not like it, so I ask for the opinion about it.
> > >
> > My thought is to standardize in the build and then leave it to module
> > owners to either change their macros to use those standard ones as time
> > goes on.
>
> In order to maintain a good global user experience,
> we need to drive such change with a roadmap and deadlines.
>
> > > >> Should they? Or should we leave the current debug macros? Please share
> > > >> your opinions as I see both cons and pros of this idea.
> > > > I am not sure we need to keep fine-grain debug flags per libs/drivers.
> > > > In case RTE_DEBUG is enabled, which kind of debug processing
> > > > (except logs) do we want to be able to disable?
> > > > Is it possible to decide based on a call to rte_log_can_log()?
> > > I think it's rather opposite way round. Sometimes we would like to
> > > enable just some debug processing, e.g. when working on single lib or
> > > driver.
> > > If we will use rte_debug - every debug processing would be enabled, we
> > > won't disable anything, but without rte_debug we will still have the
> > > possibility of enabling debugs on a single module.
> > >
> > > I believe it is possible to do it with rte_log_can_log, but changing
> > > build time enabled options into runtime enabled options might affect
> > > performance. It might make working on a single library or driver harder.
> > >
> >
> > I think the idea is to use both. When RTE_DEBUG is enabled, then the
> > rte_log_can_log() calls will be used to control the actual output. Without
> > RTE_DEBUG, the whole block is skipped.
> >
> > #ifdef RTE_DEBUG
> > if rte_log_can_log(...) {
> > /* do debug stuff */
> > }
> > #endif
>
> This is what I had in mind.
> The question is about performance impact in the case
> we enable RTE_DEBUG at compilation time, and don't enable a
> specific debug at runtime: is this check overhead acceptable?
> if rte_log_can_log(...)
We probably wouldn't know the answer before trying.
Probably best way to make such changes for rte_mbuf and see how
big the drop will be.
I suppose mbuf will be the one mostly affected,
as we'll call rte_log_can_log() for nearly every mbuf op (free/detach/attach/reset, etc.)
W dniu 22.04.2020 o 13:02, Thomas Monjalon pisze:
> 22/04/2020 12:55, Bruce Richardson:
>> On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
>>> W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
>>>> 21/04/2020 22:58, Lukasz Wojciechowski:
>>>>> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
>>>>>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
>>>>>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
>>>>>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
>>>>>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
>>>>>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
>>>>>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
>>>>>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
>>>>>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
>>>>>>>>>>>>>> layer meson.build.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
>>>>>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
>>>>>>>>>>>>>> CFLAGS="-D..."
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Konstantin
>>>>>>>>>>>>>>
>>>>>>>>>>>>> That seems a reasonable idea to me.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
>>>>>>>>>>>>> either:
>>>>>>>>>>>>>
>>>>>>>>>>>>> * allow each component meson.build file define its own flags after
>>>>>>>>>>>>> checking get_option('debug') * have lib/meson.build and
>>>>>>>>>>>>> drivers/meson.build automatically define a specific define for each
>>>>>>>>>>>>> library or driver to standardize the naming. [This would save anyone
>>>>>>>>>>>>> working on it from having to lookup what the define was, since it's
>>>>>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
>>>>>>>>>>>>> RTE_DEBUG_SCHED etc]
>>>>>>>>>>>>>
>>>>>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
>>>>>>>>>>>>> then allow a component to provide extra flags itself if so desired.
>>>>>>>>>>>>>
>>>>>>>>>>>>> /Bruce
>>>>>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
>>>>>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
>>>>>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
>>>>>>>>>>>> the RTE_DEBUG_... flags
>>>>>>>>>>>>
>>>>>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
>>>>>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
>>>>>>>>>>>> component
>>>>>>>>>>>>
>>>>>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
>>>>>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
>>>>>>>>>>>> object and written then later to rte_build_config.h before compilation
>>>>>>>>>>>> stage. All the other modules will be able to use these flags.
>>>>>>>>>>>>
>>>>>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
>>>>>>>>>>> others are ok with this before you spend too much effort implementing it.
>>>>>>>>>>>
>>>>>>>>>>> For the drivers, the flag probably needs to include the category as well as
>>>>>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
>>>>>>>>>>> name confusion. Those flags can then be checked inside individual
>>>>>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
>>>>>>>>>>> you could theoretically do:
>>>>>>>>>>>
>>>>>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
>>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
>>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
>>>>>>>>>>> ...
>>>>>>>>>>> endif
>>>>>>>>>>>
>>>>>>>>>>> to enable more fine-grained control if so desired, and to maintain
>>>>>>>>>>> compatibility with existing defines, again if so desired.
>>>>>>>>>> Nak the nak from Cristian.
>>>>>>>>>>
>>>>>>>>>> We don't need all these flags.
>>>>>>>>>> If the user choose to compile DPDK for debug, every debug facilities
>>>>>>>>>> should be enabled. Then at runtime it is possible to enable/disable
>>>>>>>>>> the interesting logs.
>>>>>>>>>> If you need to disable something which is not a log,
>>>>>>>>>> you can align with the log level thanks to the function rte_log_can_log.
>>>>>> For many libs these flags mean much more than just logging.
>>>>>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
>>>>>> drivers - extra validation performed.
>>>>>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
>>>>>> rte_mbuf_sanity_check() instead of just NOP.
>>>>>> Which means performance would be greatly affected.
>>>>>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
>>>>>> and enforce extra checking, stats collection.
>>>>>> etc.
>>>>>> Probably that's ok for some cases to enable all that extra validation we have at once.
>>>>>> But I suppose in many cases people just interested to enable debug on one
>>>>>> (ok might be two/three) particular libraries, not the whole system.
>>>>>> Right now there is such ability, we are going to remove it without
>>>>>> providing adequate replacement.
>>>>>> Approach with rte_log_can_log() seems workable,
>>>>>> but right now these patches don't implement it.
>>>>>> Konstantin
>>>>>>
>>>>>>>>>> Please let's stop complicating things for the contributors and the users.
>>>>>>>>>> Note: I am strong on this position.
>>>>>>>>>>
>>>>>>>>> Note, this means that you need to ensure all debug printouts from libs and
>>>>>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
>>>>>>>>> that may be some distance from reality right now.
>>>>>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
>>>>>>>> And next step is to block any patch in those drivers or libs,
>>>>>>>> until it is fixed. Dynamic logging should have been complete for long.
>>>>>>>>
>>>>>>> I can live with that, I suppose. Do we have any idea of the magnitude of
>>>>>>> the work required here?
>>>>>>>
>>>>>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
>>>>>>>>> convinced that the existing debug flag is the way to do, since it only adds
>>>>>>>>> debug info to binary without affecting code generation.
>>>>>>>> OK, we want to keep this flag for gdb symbols only?
>>>>>>>> And add a new flag for debugging facilities which hurt the runtime performance?
>>>>>>>>
>>>>>>> I think that would be wise, yes. We can call the option "rte_debug" or
>>>>>>> something instead.
>>>>>>>
>>>>>>> /Bruce
>>>>> OK, lets's summarize current opinions and requirements to make a
>>>>> proposal for version2 of these patches, that I can implement if all agree:
>>>>>
>>>>> 1) The global debug flag is required to enable all the sanity checks and
>>>>> validations that are normally not used due to performance reasons
>>>> Yes
>>>>
>>>>> 2) The best option would be to have a single flag - not to introduce too
>>>>> many build options
>>>> Yes
>>>>
>>>>> 3) This option should be separated from meson "debug" option (used for
>>>>> build with symbols) and can be called "rte_debug"
>>>> Yes, it looks to be the consensus.
>>>>
>>>>> 4) The currently existing DEBUG macros should not be replaced with a
>>>>> RTE_DEBUG macro. This would allow to still enable them using
>>>>> CFLAGS="-D..." to test a single module (library, driver).
>>>>>
>>>>> 5) Currently existing options' names should be standardized to
>>>>> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
>>>>> when rte_debug is set. Standardized names would allow easy usage in
>>>>> other modules.
>>>> I don't understand difference between 4) et 5).
>>> In current version of patches, I replaced all the DEBUG macros with
>>> RTE_DEBUG. It would be much better to keep fine-grained debugs as they
>>> are defined currently in dpdk. This is what I have on mind in 4)
>>>
>>> However the currently used debug macros have different naming
>>> conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
>>> RTE_{name}_DEBUG, some just {name}_DEBUG.
>>> So in 5) I follow Bruce's proposal to standardize them to one form
>>> RTE_DEBUG_{name}. However this will change the existing macros and
>>> someone might not like it, so I ask for the opinion about it.
>>>
>> My thought is to standardize in the build and then leave it to module
>> owners to either change their macros to use those standard ones as time
>> goes on.
> In order to maintain a good global user experience,
> we need to drive such change with a roadmap and deadlines.
What is the process of documenting new wanted feature and adding it the
roadmap?
>
>>>>> Should they? Or should we leave the current debug macros? Please share
>>>>> your opinions as I see both cons and pros of this idea.
>>>> I am not sure we need to keep fine-grain debug flags per libs/drivers.
>>>> In case RTE_DEBUG is enabled, which kind of debug processing
>>>> (except logs) do we want to be able to disable?
>>>> Is it possible to decide based on a call to rte_log_can_log()?
>>> I think it's rather opposite way round. Sometimes we would like to
>>> enable just some debug processing, e.g. when working on single lib or
>>> driver.
>>> If we will use rte_debug - every debug processing would be enabled, we
>>> won't disable anything, but without rte_debug we will still have the
>>> possibility of enabling debugs on a single module.
>>>
>>> I believe it is possible to do it with rte_log_can_log, but changing
>>> build time enabled options into runtime enabled options might affect
>>> performance. It might make working on a single library or driver harder.
>>>
>> I think the idea is to use both. When RTE_DEBUG is enabled, then the
>> rte_log_can_log() calls will be used to control the actual output. Without
>> RTE_DEBUG, the whole block is skipped.
>>
>> #ifdef RTE_DEBUG
>> if rte_log_can_log(...) {
>> /* do debug stuff */
>> }
>> #endif
I thought that we are closer to agree to remain old macros, like:
#ifdef RET_DEBUG_SOMELIBRARY
if rte_log_can_log(...) {
with enabling RET_DEBUG_SOMELIBRARY in general library meson.build when
rte_debug option is set.
> This is what I had in mind.
> The question is about performance impact in the case
> we enable RTE_DEBUG at compilation time, and don't enable a
> specific debug at runtime: is this check overhead acceptable?
> if rte_log_can_log(...)
I guess that with rte_debug enabled you must accept drop of performance,
but let libraries and drivers maintainers share their opinion.
>
>
>
22/04/2020 13:29, Ananyev, Konstantin:
> > 22/04/2020 12:55, Bruce Richardson:
> > > On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> > > > W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > > > > 21/04/2020 22:58, Lukasz Wojciechowski:
> > > > >> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> > > > >>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> > > > >>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> > > > >>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> > > > >>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > > >>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> > > > >>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > > >>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > > >>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> > > > >>>>>>>>>>> layer meson.build.
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> > > > >>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> > > > >>>>>>>>>>> CFLAGS="-D..."
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>>> Konstantin
> > > > >>>>>>>>>>>
> > > > >>>>>>>>>> That seems a reasonable idea to me.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > >>>>>>>>>> either:
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> * allow each component meson.build file define its own flags after
> > > > >>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> > > > >>>>>>>>>> drivers/meson.build automatically define a specific define for each
> > > > >>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> > > > >>>>>>>>>> working on it from having to lookup what the define was, since it's
> > > > >>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> > > > >>>>>>>>>> RTE_DEBUG_SCHED etc]
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> > > > >>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> > > > >>>>>>>>>>
> > > > >>>>>>>>>> /Bruce
> > > > >>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> > > > >>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> > > > >>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > > >>>>>>>>> the RTE_DEBUG_... flags
> > > > >>>>>>>>>
> > > > >>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> > > > >>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> > > > >>>>>>>>> component
> > > > >>>>>>>>>
> > > > >>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> > > > >>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> > > > >>>>>>>>> object and written then later to rte_build_config.h before compilation
> > > > >>>>>>>>> stage. All the other modules will be able to use these flags.
> > > > >>>>>>>>>
> > > > >>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > > >>>>>>>> others are ok with this before you spend too much effort implementing it.
> > > > >>>>>>>>
> > > > >>>>>>>> For the drivers, the flag probably needs to include the category as well as
> > > > >>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > > >>>>>>>> name confusion. Those flags can then be checked inside individual
> > > > >>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > > >>>>>>>> you could theoretically do:
> > > > >>>>>>>>
> > > > >>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > > >>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > > >>>>>>>> ...
> > > > >>>>>>>> endif
> > > > >>>>>>>>
> > > > >>>>>>>> to enable more fine-grained control if so desired, and to maintain
> > > > >>>>>>>> compatibility with existing defines, again if so desired.
> > > > >>>>>>> Nak the nak from Cristian.
> > > > >>>>>>>
> > > > >>>>>>> We don't need all these flags.
> > > > >>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> > > > >>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> > > > >>>>>>> the interesting logs.
> > > > >>>>>>> If you need to disable something which is not a log,
> > > > >>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> > > > >>> For many libs these flags mean much more than just logging.
> > > > >>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > > > >>> drivers - extra validation performed.
> > > > >>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> > > > >>> rte_mbuf_sanity_check() instead of just NOP.
> > > > >>> Which means performance would be greatly affected.
> > > > >>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > > > >>> and enforce extra checking, stats collection.
> > > > >>> etc.
> > > > >>> Probably that's ok for some cases to enable all that extra validation we have at once.
> > > > >>> But I suppose in many cases people just interested to enable debug on one
> > > > >>> (ok might be two/three) particular libraries, not the whole system.
> > > > >>> Right now there is such ability, we are going to remove it without
> > > > >>> providing adequate replacement.
> > > > >>> Approach with rte_log_can_log() seems workable,
> > > > >>> but right now these patches don't implement it.
> > > > >>> Konstantin
> > > > >>>
> > > > >>>>>>> Please let's stop complicating things for the contributors and the users.
> > > > >>>>>>> Note: I am strong on this position.
> > > > >>>>>>>
> > > > >>>>>> Note, this means that you need to ensure all debug printouts from libs and
> > > > >>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > > > >>>>>> that may be some distance from reality right now.
> > > > >>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> > > > >>>>> And next step is to block any patch in those drivers or libs,
> > > > >>>>> until it is fixed. Dynamic logging should have been complete for long.
> > > > >>>>>
> > > > >>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> > > > >>>> the work required here?
> > > > >>>>
> > > > >>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> > > > >>>>>> convinced that the existing debug flag is the way to do, since it only adds
> > > > >>>>>> debug info to binary without affecting code generation.
> > > > >>>>> OK, we want to keep this flag for gdb symbols only?
> > > > >>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> > > > >>>>>
> > > > >>>> I think that would be wise, yes. We can call the option "rte_debug" or
> > > > >>>> something instead.
> > > > >>>>
> > > > >>>> /Bruce
> > > > >> OK, lets's summarize current opinions and requirements to make a
> > > > >> proposal for version2 of these patches, that I can implement if all agree:
> > > > >>
> > > > >> 1) The global debug flag is required to enable all the sanity checks and
> > > > >> validations that are normally not used due to performance reasons
> > > > > Yes
> > > > >
> > > > >> 2) The best option would be to have a single flag - not to introduce too
> > > > >> many build options
> > > > > Yes
> > > > >
> > > > >> 3) This option should be separated from meson "debug" option (used for
> > > > >> build with symbols) and can be called "rte_debug"
> > > > > Yes, it looks to be the consensus.
> > > > >
> > > > >> 4) The currently existing DEBUG macros should not be replaced with a
> > > > >> RTE_DEBUG macro. This would allow to still enable them using
> > > > >> CFLAGS="-D..." to test a single module (library, driver).
> > > > >>
> > > > >> 5) Currently existing options' names should be standardized to
> > > > >> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> > > > >> when rte_debug is set. Standardized names would allow easy usage in
> > > > >> other modules.
> > > > > I don't understand difference between 4) et 5).
> > > >
> > > > In current version of patches, I replaced all the DEBUG macros with
> > > > RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> > > > are defined currently in dpdk. This is what I have on mind in 4)
> > > >
> > > > However the currently used debug macros have different naming
> > > > conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> > > > RTE_{name}_DEBUG, some just {name}_DEBUG.
> > > > So in 5) I follow Bruce's proposal to standardize them to one form
> > > > RTE_DEBUG_{name}. However this will change the existing macros and
> > > > someone might not like it, so I ask for the opinion about it.
> > > >
> > > My thought is to standardize in the build and then leave it to module
> > > owners to either change their macros to use those standard ones as time
> > > goes on.
> >
> > In order to maintain a good global user experience,
> > we need to drive such change with a roadmap and deadlines.
> >
> > > > >> Should they? Or should we leave the current debug macros? Please share
> > > > >> your opinions as I see both cons and pros of this idea.
> > > > > I am not sure we need to keep fine-grain debug flags per libs/drivers.
> > > > > In case RTE_DEBUG is enabled, which kind of debug processing
> > > > > (except logs) do we want to be able to disable?
> > > > > Is it possible to decide based on a call to rte_log_can_log()?
> > > > I think it's rather opposite way round. Sometimes we would like to
> > > > enable just some debug processing, e.g. when working on single lib or
> > > > driver.
> > > > If we will use rte_debug - every debug processing would be enabled, we
> > > > won't disable anything, but without rte_debug we will still have the
> > > > possibility of enabling debugs on a single module.
> > > >
> > > > I believe it is possible to do it with rte_log_can_log, but changing
> > > > build time enabled options into runtime enabled options might affect
> > > > performance. It might make working on a single library or driver harder.
> > > >
> > >
> > > I think the idea is to use both. When RTE_DEBUG is enabled, then the
> > > rte_log_can_log() calls will be used to control the actual output. Without
> > > RTE_DEBUG, the whole block is skipped.
> > >
> > > #ifdef RTE_DEBUG
> > > if rte_log_can_log(...) {
> > > /* do debug stuff */
> > > }
> > > #endif
> >
> > This is what I had in mind.
> > The question is about performance impact in the case
> > we enable RTE_DEBUG at compilation time, and don't enable a
> > specific debug at runtime: is this check overhead acceptable?
> > if rte_log_can_log(...)
>
> We probably wouldn't know the answer before trying.
> Probably best way to make such changes for rte_mbuf and see how
> big the drop will be.
> I suppose mbuf will be the one mostly affected,
> as we'll call rte_log_can_log() for nearly every mbuf op (free/detach/attach/reset, etc.)
Yes I like the idea of trying with mbuf,
and see how much a compilation flag for global debugging
affects the mbuf performance.
Note that we should also manage RTE_ENABLE_ASSERT with the global meson option.
On Wed, Apr 22, 2020 at 01:52:21PM +0200, Lukasz Wojciechowski wrote:
>
> W dniu 22.04.2020 o 13:02, Thomas Monjalon pisze:
> > 22/04/2020 12:55, Bruce Richardson:
> >> On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> >>> W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> >>>> 21/04/2020 22:58, Lukasz Wojciechowski:
> >>>>> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> >>>>>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
> >>>>>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
> >>>>>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
> >>>>>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> >>>>>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
> >>>>>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> >>>>>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
> >>>>>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
> >>>>>>>>>>>>>> layer meson.build.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
> >>>>>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
> >>>>>>>>>>>>>> CFLAGS="-D..."
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Konstantin
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>> That seems a reasonable idea to me.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
> >>>>>>>>>>>>> either:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> * allow each component meson.build file define its own flags after
> >>>>>>>>>>>>> checking get_option('debug') * have lib/meson.build and
> >>>>>>>>>>>>> drivers/meson.build automatically define a specific define for each
> >>>>>>>>>>>>> library or driver to standardize the naming. [This would save anyone
> >>>>>>>>>>>>> working on it from having to lookup what the define was, since it's
> >>>>>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
> >>>>>>>>>>>>> RTE_DEBUG_SCHED etc]
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
> >>>>>>>>>>>>> then allow a component to provide extra flags itself if so desired.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> /Bruce
> >>>>>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
> >>>>>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
> >>>>>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
> >>>>>>>>>>>> the RTE_DEBUG_... flags
> >>>>>>>>>>>>
> >>>>>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
> >>>>>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
> >>>>>>>>>>>> component
> >>>>>>>>>>>>
> >>>>>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
> >>>>>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
> >>>>>>>>>>>> object and written then later to rte_build_config.h before compilation
> >>>>>>>>>>>> stage. All the other modules will be able to use these flags.
> >>>>>>>>>>>>
> >>>>>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
> >>>>>>>>>>> others are ok with this before you spend too much effort implementing it.
> >>>>>>>>>>>
> >>>>>>>>>>> For the drivers, the flag probably needs to include the category as well as
> >>>>>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> >>>>>>>>>>> name confusion. Those flags can then be checked inside individual
> >>>>>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> >>>>>>>>>>> you could theoretically do:
> >>>>>>>>>>>
> >>>>>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> >>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> >>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> >>>>>>>>>>> ...
> >>>>>>>>>>> endif
> >>>>>>>>>>>
> >>>>>>>>>>> to enable more fine-grained control if so desired, and to maintain
> >>>>>>>>>>> compatibility with existing defines, again if so desired.
> >>>>>>>>>> Nak the nak from Cristian.
> >>>>>>>>>>
> >>>>>>>>>> We don't need all these flags.
> >>>>>>>>>> If the user choose to compile DPDK for debug, every debug facilities
> >>>>>>>>>> should be enabled. Then at runtime it is possible to enable/disable
> >>>>>>>>>> the interesting logs.
> >>>>>>>>>> If you need to disable something which is not a log,
> >>>>>>>>>> you can align with the log level thanks to the function rte_log_can_log.
> >>>>>> For many libs these flags mean much more than just logging.
> >>>>>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> >>>>>> drivers - extra validation performed.
> >>>>>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
> >>>>>> rte_mbuf_sanity_check() instead of just NOP.
> >>>>>> Which means performance would be greatly affected.
> >>>>>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> >>>>>> and enforce extra checking, stats collection.
> >>>>>> etc.
> >>>>>> Probably that's ok for some cases to enable all that extra validation we have at once.
> >>>>>> But I suppose in many cases people just interested to enable debug on one
> >>>>>> (ok might be two/three) particular libraries, not the whole system.
> >>>>>> Right now there is such ability, we are going to remove it without
> >>>>>> providing adequate replacement.
> >>>>>> Approach with rte_log_can_log() seems workable,
> >>>>>> but right now these patches don't implement it.
> >>>>>> Konstantin
> >>>>>>
> >>>>>>>>>> Please let's stop complicating things for the contributors and the users.
> >>>>>>>>>> Note: I am strong on this position.
> >>>>>>>>>>
> >>>>>>>>> Note, this means that you need to ensure all debug printouts from libs and
> >>>>>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
> >>>>>>>>> that may be some distance from reality right now.
> >>>>>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> >>>>>>>> And next step is to block any patch in those drivers or libs,
> >>>>>>>> until it is fixed. Dynamic logging should have been complete for long.
> >>>>>>>>
> >>>>>>> I can live with that, I suppose. Do we have any idea of the magnitude of
> >>>>>>> the work required here?
> >>>>>>>
> >>>>>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
> >>>>>>>>> convinced that the existing debug flag is the way to do, since it only adds
> >>>>>>>>> debug info to binary without affecting code generation.
> >>>>>>>> OK, we want to keep this flag for gdb symbols only?
> >>>>>>>> And add a new flag for debugging facilities which hurt the runtime performance?
> >>>>>>>>
> >>>>>>> I think that would be wise, yes. We can call the option "rte_debug" or
> >>>>>>> something instead.
> >>>>>>>
> >>>>>>> /Bruce
> >>>>> OK, lets's summarize current opinions and requirements to make a
> >>>>> proposal for version2 of these patches, that I can implement if all agree:
> >>>>>
> >>>>> 1) The global debug flag is required to enable all the sanity checks and
> >>>>> validations that are normally not used due to performance reasons
> >>>> Yes
> >>>>
> >>>>> 2) The best option would be to have a single flag - not to introduce too
> >>>>> many build options
> >>>> Yes
> >>>>
> >>>>> 3) This option should be separated from meson "debug" option (used for
> >>>>> build with symbols) and can be called "rte_debug"
> >>>> Yes, it looks to be the consensus.
> >>>>
> >>>>> 4) The currently existing DEBUG macros should not be replaced with a
> >>>>> RTE_DEBUG macro. This would allow to still enable them using
> >>>>> CFLAGS="-D..." to test a single module (library, driver).
> >>>>>
> >>>>> 5) Currently existing options' names should be standardized to
> >>>>> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
> >>>>> when rte_debug is set. Standardized names would allow easy usage in
> >>>>> other modules.
> >>>> I don't understand difference between 4) et 5).
> >>> In current version of patches, I replaced all the DEBUG macros with
> >>> RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> >>> are defined currently in dpdk. This is what I have on mind in 4)
> >>>
> >>> However the currently used debug macros have different naming
> >>> conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> >>> RTE_{name}_DEBUG, some just {name}_DEBUG.
> >>> So in 5) I follow Bruce's proposal to standardize them to one form
> >>> RTE_DEBUG_{name}. However this will change the existing macros and
> >>> someone might not like it, so I ask for the opinion about it.
> >>>
> >> My thought is to standardize in the build and then leave it to module
> >> owners to either change their macros to use those standard ones as time
> >> goes on.
> > In order to maintain a good global user experience,
> > we need to drive such change with a roadmap and deadlines.
> What is the process of documenting new wanted feature and adding it the
> roadmap?
> >
> >>>>> Should they? Or should we leave the current debug macros? Please share
> >>>>> your opinions as I see both cons and pros of this idea.
> >>>> I am not sure we need to keep fine-grain debug flags per libs/drivers.
> >>>> In case RTE_DEBUG is enabled, which kind of debug processing
> >>>> (except logs) do we want to be able to disable?
> >>>> Is it possible to decide based on a call to rte_log_can_log()?
> >>> I think it's rather opposite way round. Sometimes we would like to
> >>> enable just some debug processing, e.g. when working on single lib or
> >>> driver.
> >>> If we will use rte_debug - every debug processing would be enabled, we
> >>> won't disable anything, but without rte_debug we will still have the
> >>> possibility of enabling debugs on a single module.
> >>>
> >>> I believe it is possible to do it with rte_log_can_log, but changing
> >>> build time enabled options into runtime enabled options might affect
> >>> performance. It might make working on a single library or driver harder.
> >>>
> >> I think the idea is to use both. When RTE_DEBUG is enabled, then the
> >> rte_log_can_log() calls will be used to control the actual output. Without
> >> RTE_DEBUG, the whole block is skipped.
> >>
> >> #ifdef RTE_DEBUG
> >> if rte_log_can_log(...) {
> >> /* do debug stuff */
> >> }
> >> #endif
> I thought that we are closer to agree to remain old macros, like:
>
> #ifdef RET_DEBUG_SOMELIBRARY
> if rte_log_can_log(...) {
>
> with enabling RET_DEBUG_SOMELIBRARY in general library meson.build when
> rte_debug option is set.
>
Yes, looks better to me, since it does allow use of CFLAGS if you *really*
want to limit the debug to just one module.
W dniu 22.04.2020 o 14:24, Thomas Monjalon pisze:
> 22/04/2020 13:29, Ananyev, Konstantin:
>>> 22/04/2020 12:55, Bruce Richardson:
>>>> On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
>>>>> W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
>>>>>> 21/04/2020 22:58, Lukasz Wojciechowski:
>>>>>>> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
>>>>>>>>>>>>>>>> I am agree with Cristian concern here: that patch removes ability to
>>>>>>>>>>>>>>>> enable/disable debug on particular library/PMD. If the purpose is to
>>>>>>>>>>>>>>>> minimize number of config compile options, I wonder can't it be done
>>>>>>>>>>>>>>>> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
>>>>>>>>>>>>>>>> actual .[c,h] files intact 3. In actual librte_xxx/meson.build file
>>>>>>>>>>>>>>>> check if RTE_DEBUG is enabled, If yes, then enable particular debug
>>>>>>>>>>>>>>>> flag for these libs. Something like: If dpdk_conf.get('RTE_DEBUG') ==
>>>>>>>>>>>>>>>> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> defines that are used by multiple libs, probably can be set in upper
>>>>>>>>>>>>>>>> layer meson.build.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> That way will have global 'debug' flag, but users will still have an
>>>>>>>>>>>>>>>> ability to enable/disable debug flags on a per lib basis via
>>>>>>>>>>>>>>>> CFLAGS="-D..."
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Konstantin
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> That seems a reasonable idea to me.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> However, in this case, we don't need the RTE_DEBUG flag at all, we can
>>>>>>>>>>>>>>> either:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> * allow each component meson.build file define its own flags after
>>>>>>>>>>>>>>> checking get_option('debug') * have lib/meson.build and
>>>>>>>>>>>>>>> drivers/meson.build automatically define a specific define for each
>>>>>>>>>>>>>>> library or driver to standardize the naming. [This would save anyone
>>>>>>>>>>>>>>> working on it from having to lookup what the define was, since it's
>>>>>>>>>>>>>>> always e.g. RTE_DEBUG_ + library-base-name, e.g. RTE_DEBUG_LPM,
>>>>>>>>>>>>>>> RTE_DEBUG_SCHED etc]
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Theoretically we can also do both, have the standard ones defined and
>>>>>>>>>>>>>>> then allow a component to provide extra flags itself if so desired.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> /Bruce
>>>>>>>>>>>>>> OK, so let's summarize how the patches should be redo: * usage of global
>>>>>>>>>>>>>> "debug" flag for meson build stays * we standardize names of debug flags
>>>>>>>>>>>>>> in the components to RTE_DEBUG_ + components name * debug flag enables al
>>>>>>>>>>>>>> the RTE_DEBUG_... flags
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This allow to easily use both: * the debug flag - to enable all debugs *
>>>>>>>>>>>>>> or define manually RTE_DEBUG+component name, just for debug from a single
>>>>>>>>>>>>>> component
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I like Bruce's idea of adding it to the lib/meson.build and
>>>>>>>>>>>>>> drivers/meson.build. This way they will be added to dpdk_conf meson
>>>>>>>>>>>>>> object and written then later to rte_build_config.h before compilation
>>>>>>>>>>>>>> stage. All the other modules will be able to use these flags.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Sounds good to me (obviously!), but I'd like other feedback to ensure
>>>>>>>>>>>>> others are ok with this before you spend too much effort implementing it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> For the drivers, the flag probably needs to include the category as well as
>>>>>>>>>>>>> the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
>>>>>>>>>>>>> name confusion. Those flags can then be checked inside individual
>>>>>>>>>>>>> meson.build files to enable other debug flags if necessary e.g. in ixgbe,
>>>>>>>>>>>>> you could theoretically do:
>>>>>>>>>>>>>
>>>>>>>>>>>>> if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
>>>>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
>>>>>>>>>>>>> cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
>>>>>>>>>>>>> ...
>>>>>>>>>>>>> endif
>>>>>>>>>>>>>
>>>>>>>>>>>>> to enable more fine-grained control if so desired, and to maintain
>>>>>>>>>>>>> compatibility with existing defines, again if so desired.
>>>>>>>>>>>> Nak the nak from Cristian.
>>>>>>>>>>>>
>>>>>>>>>>>> We don't need all these flags.
>>>>>>>>>>>> If the user choose to compile DPDK for debug, every debug facilities
>>>>>>>>>>>> should be enabled. Then at runtime it is possible to enable/disable
>>>>>>>>>>>> the interesting logs.
>>>>>>>>>>>> If you need to disable something which is not a log,
>>>>>>>>>>>> you can align with the log level thanks to the function rte_log_can_log.
>>>>>>>> For many libs these flags mean much more than just logging.
>>>>>>>> Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
>>>>>>>> drivers - extra validation performed.
>>>>>>>> RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call to real
>>>>>>>> rte_mbuf_sanity_check() instead of just NOP.
>>>>>>>> Which means performance would be greatly affected.
>>>>>>>> RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
>>>>>>>> and enforce extra checking, stats collection.
>>>>>>>> etc.
>>>>>>>> Probably that's ok for some cases to enable all that extra validation we have at once.
>>>>>>>> But I suppose in many cases people just interested to enable debug on one
>>>>>>>> (ok might be two/three) particular libraries, not the whole system.
>>>>>>>> Right now there is such ability, we are going to remove it without
>>>>>>>> providing adequate replacement.
>>>>>>>> Approach with rte_log_can_log() seems workable,
>>>>>>>> but right now these patches don't implement it.
>>>>>>>> Konstantin
>>>>>>>>
>>>>>>>>>>>> Please let's stop complicating things for the contributors and the users.
>>>>>>>>>>>> Note: I am strong on this position.
>>>>>>>>>>>>
>>>>>>>>>>> Note, this means that you need to ensure all debug printouts from libs and
>>>>>>>>>>> drivers are using the RTE_LOG macros so can be runtime controlled. I think
>>>>>>>>>>> that may be some distance from reality right now.
>>>>>>>>>> Perfect! Let's expose those nasty logs which are not (yet) controllable.
>>>>>>>>>> And next step is to block any patch in those drivers or libs,
>>>>>>>>>> until it is fixed. Dynamic logging should have been complete for long.
>>>>>>>>>>
>>>>>>>>> I can live with that, I suppose. Do we have any idea of the magnitude of
>>>>>>>>> the work required here?
>>>>>>>>>
>>>>>>>>>>> Even if we do want all debug enabled from one flag, I'm still not 100%
>>>>>>>>>>> convinced that the existing debug flag is the way to do, since it only adds
>>>>>>>>>>> debug info to binary without affecting code generation.
>>>>>>>>>> OK, we want to keep this flag for gdb symbols only?
>>>>>>>>>> And add a new flag for debugging facilities which hurt the runtime performance?
>>>>>>>>>>
>>>>>>>>> I think that would be wise, yes. We can call the option "rte_debug" or
>>>>>>>>> something instead.
>>>>>>>>>
>>>>>>>>> /Bruce
>>>>>>> OK, lets's summarize current opinions and requirements to make a
>>>>>>> proposal for version2 of these patches, that I can implement if all agree:
>>>>>>>
>>>>>>> 1) The global debug flag is required to enable all the sanity checks and
>>>>>>> validations that are normally not used due to performance reasons
>>>>>> Yes
>>>>>>
>>>>>>> 2) The best option would be to have a single flag - not to introduce too
>>>>>>> many build options
>>>>>> Yes
>>>>>>
>>>>>>> 3) This option should be separated from meson "debug" option (used for
>>>>>>> build with symbols) and can be called "rte_debug"
>>>>>> Yes, it looks to be the consensus.
>>>>>>
>>>>>>> 4) The currently existing DEBUG macros should not be replaced with a
>>>>>>> RTE_DEBUG macro. This would allow to still enable them using
>>>>>>> CFLAGS="-D..." to test a single module (library, driver).
>>>>>>>
>>>>>>> 5) Currently existing options' names should be standardized to
>>>>>>> RTE_DEBUG_{library/driver name}, so they can be automatically enabled
>>>>>>> when rte_debug is set. Standardized names would allow easy usage in
>>>>>>> other modules.
>>>>>> I don't understand difference between 4) et 5).
>>>>> In current version of patches, I replaced all the DEBUG macros with
>>>>> RTE_DEBUG. It would be much better to keep fine-grained debugs as they
>>>>> are defined currently in dpdk. This is what I have on mind in 4)
>>>>>
>>>>> However the currently used debug macros have different naming
>>>>> conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
>>>>> RTE_{name}_DEBUG, some just {name}_DEBUG.
>>>>> So in 5) I follow Bruce's proposal to standardize them to one form
>>>>> RTE_DEBUG_{name}. However this will change the existing macros and
>>>>> someone might not like it, so I ask for the opinion about it.
>>>>>
>>>> My thought is to standardize in the build and then leave it to module
>>>> owners to either change their macros to use those standard ones as time
>>>> goes on.
>>> In order to maintain a good global user experience,
>>> we need to drive such change with a roadmap and deadlines.
>>>
>>>>>>> Should they? Or should we leave the current debug macros? Please share
>>>>>>> your opinions as I see both cons and pros of this idea.
>>>>>> I am not sure we need to keep fine-grain debug flags per libs/drivers.
>>>>>> In case RTE_DEBUG is enabled, which kind of debug processing
>>>>>> (except logs) do we want to be able to disable?
>>>>>> Is it possible to decide based on a call to rte_log_can_log()?
>>>>> I think it's rather opposite way round. Sometimes we would like to
>>>>> enable just some debug processing, e.g. when working on single lib or
>>>>> driver.
>>>>> If we will use rte_debug - every debug processing would be enabled, we
>>>>> won't disable anything, but without rte_debug we will still have the
>>>>> possibility of enabling debugs on a single module.
>>>>>
>>>>> I believe it is possible to do it with rte_log_can_log, but changing
>>>>> build time enabled options into runtime enabled options might affect
>>>>> performance. It might make working on a single library or driver harder.
>>>>>
>>>> I think the idea is to use both. When RTE_DEBUG is enabled, then the
>>>> rte_log_can_log() calls will be used to control the actual output. Without
>>>> RTE_DEBUG, the whole block is skipped.
>>>>
>>>> #ifdef RTE_DEBUG
>>>> if rte_log_can_log(...) {
>>>> /* do debug stuff */
>>>> }
>>>> #endif
>>> This is what I had in mind.
>>> The question is about performance impact in the case
>>> we enable RTE_DEBUG at compilation time, and don't enable a
>>> specific debug at runtime: is this check overhead acceptable?
>>> if rte_log_can_log(...)
>> We probably wouldn't know the answer before trying.
>> Probably best way to make such changes for rte_mbuf and see how
>> big the drop will be.
>> I suppose mbuf will be the one mostly affected,
>> as we'll call rte_log_can_log() for nearly every mbuf op (free/detach/attach/reset, etc.)
> Yes I like the idea of trying with mbuf,
> and see how much a compilation flag for global debugging
> affects the mbuf performance.
I've just pushed v3 with minor fixes and mbuf performance tests. There
are 6 very simple tests:
* alloc_free allocates and frees mbufs from/to mempool one by one
* bulk_alloc_free does the same but in bulks
* data_manipulation uses few functions containing sanity checks
(is_contiguous, append, trim, prepend, adj)
* sanity_checks_without_header runs robust sanity checks (with header
parameter = 0)
* sanity_checks_with_header does full sanity checks (with header
parameter = 1)
* sanity_checks_with_header_in_chain does the same as above, but all
mbufs are chained into single list.
I run those tests in 3 different compilations:
* debug without_rte_log_can_log - sanity checks not empty, but without
runtime checks of rte_log_can_log
* debug with_rte_log_can_log - debug enabled and runtime checks used
* no debug - debug mode disabled - sanity checks are empty macros
All the tests use 1024 mbufs of size (2048 + 128) and repeat test 1e6 times.
I repeated every scenario 5 times and calculated average times as you
can see in the below table. All times are in seconds.
There is no much difference between both debug variants. Of course
without rte_log it's slightly better, but the time deviation between
samples is very big and in many cases bigger than differences.
There is a huge difference between debug and non-debug scenarios
especially in data_manipulation test case.
So Konstantin, what do you think about these results. Can we apply
similar patches for other libraries and drivers?
> Note that we should also manage RTE_ENABLE_ASSERT with the global meson option.
>
>
>
>
Hi Lukasz,
> I've just pushed v3 with minor fixes and mbuf performance tests. There are 6 very simple tests:
> * alloc_free allocates and frees mbufs from/to mempool one by one
> * bulk_alloc_free does the same but in bulks
> * data_manipulation uses few functions containing sanity checks (is_contiguous, append, trim, prepend, adj)
> * sanity_checks_without_header runs robust sanity checks (with header parameter = 0)
> * sanity_checks_with_header does full sanity checks (with header parameter = 1)
> * sanity_checks_with_header_in_chain does the same as above, but all mbufs are chained into single list.
> I run those tests in 3 different compilations:
> * debug without_rte_log_can_log - sanity checks not empty, but without runtime checks of rte_log_can_log
> * debug with_rte_log_can_log - debug enabled and runtime checks used
> * no debug - debug mode disabled - sanity checks are empty macros
Do we have data when debug was enabled at build-time (RTE_DEBUG is on),
but was disabled at run-time (rte_log_can_log() will return zero for mbuf functions)?
That the situation I was concerned about.
Konstantin
On Mon, 20 Apr 2020 19:34:43 +0200
Lukasz Wojciechowski <l.wojciechow@partner.samsung.com> wrote:
> > Note, this means that you need to ensure all debug printouts from libs and
> > drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > that may be some distance from reality right now.
> >
> > Even if we do want all debug enabled from one flag, I'm still not 100%
> > convinced that the existing debug flag is the way to do, since it only adds
> > debug info to binary without affecting code generation.
> OK, I see that there are different opinions on what shape the debug flag
> should look like.
> So I think, I'll hold on work on any implementation until we all agree,
> what do we want.
>
> @Bruce: What code generation do you have on mind?
This patchset seems stagnated by good intentions but no later effort.
Agree that having:
- a global flag for all debugs enabled would be good.
- CI should do a build with that flag turned on.
This will check that debug code actually works and is not so verbose that
it can't run (which is often the case now).
But current patchset doesn't do that. So leaving final disposition
as "Changes requested" and someone can pick it up if they want to continue.
@@ -149,7 +149,6 @@ CONFIG_RTE_LIBRTE_KVARGS=y
# Compile generic ethernet library
#
CONFIG_RTE_LIBRTE_ETHER=y
-CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
CONFIG_RTE_MAX_ETHPORTS=32
CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
CONFIG_RTE_LIBRTE_IEEE1588=n
@@ -821,7 +821,7 @@ atl_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -626,7 +626,7 @@ eth_em_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -641,7 +641,7 @@ eth_igb_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -2145,7 +2145,7 @@ eth_ena_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -414,7 +414,7 @@ uint16_t enic_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
rte_errno = ENOTSUP;
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -710,7 +710,7 @@ fm10k_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -804,7 +804,7 @@ hinic_tx_offload_pkt_prepare(struct rte_mbuf *m,
!(ol_flags & PKT_TX_TUNNEL_VXLAN))
return -ENOTSUP;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
if (rte_validate_tx_offload(m) != 0)
return -EINVAL;
#endif
@@ -2296,7 +2296,7 @@ hns3_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -1499,7 +1499,7 @@ i40e_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -1689,7 +1689,7 @@ iavf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -3037,7 +3037,7 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -992,7 +992,7 @@ ixgbe_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -2156,7 +2156,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq, struct rte_mbuf **tx_pkts,
uint64_t ol_flags;
struct rte_mbuf *m;
uint16_t i;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
int ret;
#endif
@@ -2196,7 +2196,7 @@ qede_xmit_prep_pkts(__rte_unused void *p_txq, struct rte_mbuf **tx_pkts,
break;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -704,7 +704,7 @@ rte_pmd_softnic_manage(uint16_t port_id)
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
struct pmd_internals *softnic;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
#endif
@@ -3093,7 +3093,7 @@ rte_pmd_softnic_run(uint16_t port_id)
{
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
#endif
@@ -1979,7 +1979,7 @@ virtio_xmit_pkts_prepare(void *tx_queue __rte_unused, struct rte_mbuf **tx_pkts,
for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
struct rte_mbuf *m = tx_pkts[nb_tx];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
error = rte_validate_tx_offload(m);
if (unlikely(error)) {
rte_errno = -error;
@@ -373,7 +373,7 @@ vmxnet3_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
return i;
}
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
ret = rte_validate_tx_offload(m);
if (ret != 0) {
rte_errno = -ret;
@@ -4375,7 +4375,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
uint16_t nb_rx;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
@@ -4498,11 +4498,11 @@ rte_eth_rx_descriptor_status(uint16_t port_id, uint16_t queue_id,
struct rte_eth_dev *dev;
void *rxq;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
#endif
dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
if (queue_id >= dev->data->nb_rx_queues)
return -ENODEV;
#endif
@@ -4555,11 +4555,11 @@ static inline int rte_eth_tx_descriptor_status(uint16_t port_id,
struct rte_eth_dev *dev;
void *txq;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
#endif
dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
if (queue_id >= dev->data->nb_tx_queues)
return -ENODEV;
#endif
@@ -4641,7 +4641,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
{
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
@@ -4727,7 +4727,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
{
struct rte_eth_dev *dev;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
if (!rte_eth_dev_is_valid_port(port_id)) {
RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
rte_errno = EINVAL;
@@ -4737,7 +4737,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
if (queue_id >= dev->data->nb_tx_queues) {
RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", queue_id);
rte_errno = EINVAL;
@@ -120,7 +120,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
struct rte_udp_hdr *udp_hdr;
uint64_t inner_l3_offset = m->l2_len;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
/*
* Does packet set any of available offloads?
* Mainly it is required to avoid fragmented headers check if
@@ -133,7 +133,7 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#ifdef RTE_DEBUG
/*
* Check if headers are fragmented.
* The check could be less strict depending on which offloads are