[dpdk-dev] [PATCH] net/enic: retain previous message logging

[dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley]

Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
default. After the fix, using dynamic logging, only NOTICE level and
higher were displayed by default and INFO level were not. Change the
messages to NOTICE level so they continue to display.

DTS uses testpmd and parses messages and some tests failed because
messages were no longer displayed. Other apps may also depend on
the messages.

Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")

Signed-off-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/enic_compat.h |  2 +-
 drivers/net/enic/enic_main.c   | 25 ++++++++++++-------------
 drivers/net/enic/enic_res.c    | 10 +++++-----
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/net/enic/enic_compat.h b/drivers/net/enic/enic_compat.h
index bf9e998a6..0447bfc14 100644
--- a/drivers/net/enic/enic_compat.h
+++ b/drivers/net/enic/enic_compat.h
@@ -54,7 +54,7 @@ extern int enic_pmd_logtype;
 		"PMD: rte_enic_pmd: " fmt, ##args)
 
 #define dev_err(x, args...) dev_printk(ERR, args)
-#define dev_info(x, args...) dev_printk(INFO,  args)
+#define dev_notice(x, args...) dev_printk(NOTICE,  args)
 #define dev_warning(x, args...) dev_printk(WARNING, args)
 #define dev_debug(x, args...) dev_printk(DEBUG, args)
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 40af3781b..17331dc44 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -654,8 +654,7 @@ int enic_alloc_intr_resources(struct enic *enic)
 	int err;
 	unsigned int i;
 
-	dev_info(enic, "vNIC resources used:  "\
-		"wq %d rq %d cq %d intr %d\n",
+	dev_notice(enic, "vNIC resources used:  wq %d rq %d cq %d intr %d\n",
 		enic->wq_count, enic_vnic_rq_count(enic),
 		enic->cq_count, enic->intr_count);
 
@@ -811,11 +810,11 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 
 	if (enic->rte_dev->data->dev_conf.rxmode.offloads &
 	    DEV_RX_OFFLOAD_SCATTER) {
-		dev_info(enic, "Rq %u Scatter rx mode enabled\n", queue_idx);
+		dev_notice(enic, "Rq %u Scatter rx mode enabled\n", queue_idx);
 		/* ceil((max pkt len)/mbuf_size) */
 		mbufs_per_pkt = (max_rx_pkt_len + mbuf_size - 1) / mbuf_size;
 	} else {
-		dev_info(enic, "Scatter rx mode disabled\n");
+		dev_notice(enic, "Scatter rx mode disabled\n");
 		mbufs_per_pkt = 1;
 		if (max_rx_pkt_len > mbuf_size) {
 			dev_warning(enic, "The maximum Rx packet size (%u) is"
@@ -827,7 +826,7 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 	}
 
 	if (mbufs_per_pkt > 1) {
-		dev_info(enic, "Rq %u Scatter rx mode in use\n", queue_idx);
+		dev_notice(enic, "Rq %u Scatter rx mode in use\n", queue_idx);
 		rq_sop->data_queue_enable = 1;
 		rq_data->in_use = 1;
 		/*
@@ -843,7 +842,7 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 				    " when scatter rx mode is in use.\n");
 		}
 	} else {
-		dev_info(enic, "Rq %u Scatter rx mode not being used\n",
+		dev_notice(enic, "Rq %u Scatter rx mode not being used\n",
 			 queue_idx);
 		rq_sop->data_queue_enable = 0;
 		rq_data->in_use = 0;
@@ -881,12 +880,12 @@ int enic_alloc_rq(struct enic *enic, uint16_t queue_idx,
 		nb_data_desc = max_data;
 	}
 	if (mbufs_per_pkt > 1) {
-		dev_info(enic, "For max packet size %u and mbuf size %u valid"
+		dev_notice(enic, "For max packet size %u and mbuf size %u valid"
 			 " rx descriptor range is %u to %u\n",
 			 max_rx_pkt_len, mbuf_size, min_sop + min_data,
 			 max_sop + max_data);
 	}
-	dev_info(enic, "Using %d rx descriptors (sop %d, data %d)\n",
+	dev_notice(enic, "Using %d rx descriptors (sop %d, data %d)\n",
 		 nb_sop_desc + nb_data_desc, nb_sop_desc, nb_data_desc);
 
 	/* Allocate sop queue resources */
@@ -992,7 +991,7 @@ int enic_alloc_wq(struct enic *enic, uint16_t queue_idx,
 	 * rte_eth_tx_queue_setup() checks min, max, and alignment. So just
 	 * print an info message for diagnostics.
 	 */
-	dev_info(enic, "TX Queues - effective number of descs:%d\n", nb_desc);
+	dev_notice(enic, "TX Queues - effective number of descs:%d\n", nb_desc);
 
 	/* Allocate queue resources */
 	err = vnic_wq_alloc(enic->vdev, &enic->wq[queue_idx], queue_idx,
@@ -1518,7 +1517,7 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
 		return -EINVAL;
 	}
 	if (new_mtu < ENIC_MIN_MTU) {
-		dev_info(enic,
+		dev_notice(enic,
 			"MTU not updated: requested (%u) less than min (%u)\n",
 			new_mtu, ENIC_MIN_MTU);
 		return -EINVAL;
@@ -1609,7 +1608,7 @@ int enic_set_mtu(struct enic *enic, uint16_t new_mtu)
 	}
 
 set_mtu_done:
-	dev_info(enic, "MTU changed from %u to %u\n",  old_mtu, new_mtu);
+	dev_notice(enic, "MTU changed from %u to %u\n",  old_mtu, new_mtu);
 	rte_spinlock_unlock(&enic->mtu_lock);
 	return rc;
 }
@@ -1694,7 +1693,7 @@ static int enic_dev_init(struct enic *enic)
 						  OVERLAY_OFFLOAD_DISABLE)) {
 			dev_err(enic, "failed to disable overlay offload\n");
 		} else {
-			dev_info(enic, "Overlay offload is disabled\n");
+			dev_notice(enic, "Overlay offload is disabled\n");
 		}
 	}
 	if (!enic->disable_overlay && enic->vxlan &&
@@ -1712,7 +1711,7 @@ static int enic_dev_init(struct enic *enic)
 			PKT_TX_OUTER_IP_CKSUM |
 			PKT_TX_TUNNEL_MASK;
 		enic->overlay_offload = true;
-		dev_info(enic, "Overlay offload is enabled\n");
+		dev_notice(enic, "Overlay offload is enabled\n");
 	}
 	/*
 	 * Reset the vxlan port if HW vxlan parsing is available. It
diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
index 9405e1933..364ac527f 100644
--- a/drivers/net/enic/enic_res.c
+++ b/drivers/net/enic/enic_res.c
@@ -72,7 +72,7 @@ int enic_get_vnic_config(struct enic *enic)
 					 max_t(u16, ENIC_MIN_MTU, c->mtu));
 
 	enic->adv_filters = vnic_dev_capable_adv_filters(enic->vdev);
-	dev_info(enic, "Advanced Filters %savailable\n", ((enic->adv_filters)
+	dev_notice(enic, "Advanced Filters %savailable\n", ((enic->adv_filters)
 		 ? "" : "not "));
 
 	err = vnic_dev_capable_filter_mode(enic->vdev, &enic->flow_filter_mode,
@@ -85,7 +85,7 @@ int enic_get_vnic_config(struct enic *enic)
 	vnic_dev_capable_udp_rss_weak(enic->vdev, &enic->nic_cfg_chk,
 				      &enic->udp_rss_weak);
 
-	dev_info(enic, "Flow api filter mode: %s Actions: %s%s%s\n",
+	dev_notice(enic, "Flow api filter mode: %s Actions: %s%s%s\n",
 		((enic->flow_filter_mode == FILTER_DPDK_1) ? "DPDK" :
 		((enic->flow_filter_mode == FILTER_USNIC_IP) ? "USNIC" :
 		((enic->flow_filter_mode == FILTER_IPV4_5TUPLE) ? "5TUPLE" :
@@ -112,14 +112,14 @@ int enic_get_vnic_config(struct enic *enic)
 	c->intr_timer_usec = min_t(u32, c->intr_timer_usec,
 		vnic_dev_get_intr_coal_timer_max(enic->vdev));
 
-	dev_info(enic_get_dev(enic),
+	dev_notice(enic_get_dev(enic),
 		"vNIC MAC addr %02x:%02x:%02x:%02x:%02x:%02x "
 		"wq/rq %d/%d mtu %d, max mtu:%d\n",
 		enic->mac_addr[0], enic->mac_addr[1], enic->mac_addr[2],
 		enic->mac_addr[3], enic->mac_addr[4], enic->mac_addr[5],
 		c->wq_desc_count, c->rq_desc_count,
 		enic->rte_dev->data->mtu, enic->max_mtu);
-	dev_info(enic_get_dev(enic), "vNIC csum tx/rx %s/%s "
+	dev_notice(enic_get_dev(enic), "vNIC csum tx/rx %s/%s "
 		"rss %s intr mode %s type %s timer %d usec "
 		"loopback tag 0x%04x\n",
 		ENIC_SETTING(enic, TXCSUM) ? "yes" : "no",
@@ -268,7 +268,7 @@ void enic_get_res_counts(struct enic *enic)
 	enic->conf_intr_count = vnic_dev_get_res_count(enic->vdev,
 		RES_TYPE_INTR_CTRL);
 
-	dev_info(enic_get_dev(enic),
+	dev_notice(enic_get_dev(enic),
 		"vNIC resources avail: wq %d rq %d cq %d intr %d\n",
 		enic->conf_wq_count, enic->conf_rq_count,
 		enic->conf_cq_count, enic->conf_intr_count);
-- 
2.22.0

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[Ferruh Yigit]

On 7/25/2019 3:46 AM, John Daley wrote:
> Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> default. After the fix, using dynamic logging, only NOTICE level and
> higher were displayed by default and INFO level were not. Change the
> messages to NOTICE level so they continue to display.
> 
> DTS uses testpmd and parses messages and some tests failed because
> messages were no longer displayed. Other apps may also depend on
> the messages.

If you need messages for the test framework, why not just increase the log level
for enic PMD via application parameter [1], or as command to testpmd[2]?
Since it is dynamic debug now, you don't need to change the default, can change
the level on demand.

[1]
starting testpmd with following option should do it:
--log-level=pmd.net.enic.*:info

testpmd --log-level=pmd.net.enic.*:info -- -i


[2]
after testpmd started, can change the debug level:
testpmd> set log pmd.net.enic 7


[3] bonus, see current log levels
testpmd> dump_log_types



> 
> Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> 
> Signed-off-by: John Daley <johndale@cisco.com>

<...>

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

Ok, lets NAK this patch. See comment inline.
Thanks,
John

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, July 25, 2019 3:07 AM
> To: John Daley (johndale) <johndale@cisco.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] net/enic: retain previous message logging
> 
> On 7/25/2019 3:46 AM, John Daley wrote:
> > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > default. After the fix, using dynamic logging, only NOTICE level and
> > higher were displayed by default and INFO level were not. Change the
> > messages to NOTICE level so they continue to display.
> >
> > DTS uses testpmd and parses messages and some tests failed because
> > messages were no longer displayed. Other apps may also depend on the
> > messages.
> 
> If you need messages for the test framework, why not just increase the log
> level for enic PMD via application parameter [1], or as command to
> testpmd[2]?
> Since it is dynamic debug now, you don't need to change the default, can
> change the level on demand.

I have no problem modifying our test scripts. The bigger concern was about any other scripts out there that might break because the default enic PMD messages changed. I suppose chances are slim and any such scripts can easily be modified to set the log level to info.

> 
> [1]
> starting testpmd with following option should do it:
> --log-level=pmd.net.enic.*:info
> 
> testpmd --log-level=pmd.net.enic.*:info -- -i
> 
> 
> [2]
> after testpmd started, can change the debug level:
> testpmd> set log pmd.net.enic 7
> 
> 
> [3] bonus, see current log levels
> testpmd> dump_log_types

Nice! I didn't know about this.

> 
> 
> 
> >
> > Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> >
> > Signed-off-by: John Daley <johndale@cisco.com>
> 
> <...>

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[Hyong Youb Kim (hyonkim)]

> -----Original Message-----
> From: John Daley (johndale)
> Sent: Friday, July 26, 2019 5:26 AM
> To: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Subject: RE: [PATCH] net/enic: retain previous message logging
> 
> Ok, lets NAK this patch. See comment inline.
> Thanks,
> John
> 
[...]
> > On 7/25/2019 3:46 AM, John Daley wrote:
> > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > > default. After the fix, using dynamic logging, only NOTICE level and
> > > higher were displayed by default and INFO level were not. Change the
> > > messages to NOTICE level so they continue to display.
> > >
> > > DTS uses testpmd and parses messages and some tests failed because
> > > messages were no longer displayed. Other apps may also depend on the
> > > messages.
> >
> > If you need messages for the test framework, why not just increase the log
> > level for enic PMD via application parameter [1], or as command to
> > testpmd[2]?
> > Since it is dynamic debug now, you don't need to change the default, can
> > change the level on demand.
> 
> I have no problem modifying our test scripts. The bigger concern was about
> any other scripts out there that might break because the default enic PMD
> messages changed. I suppose chances are slim and any such scripts can easily
> be modified to set the log level to info.
> 

Hi John, Ferruh,

Can you guys reconsider? John's commit message makes it sound like he
is modifying PMD to avoid modifying test scripts. That is not the
issue at all. The real problem is that his previous commit causes
a customer visible change, which can lead to a lot of headache for both
us (doing tech support) and customers (wondering what's changed).

Prior to commit bbd8ecc05434 ("net/enic: remove PMD log type references"):

enic prints vNIC config related messages (rq/cq/wq info and such) via
dev_info(). And, dev_info() uses LOGTYPE_PMD and the INFO level.
LOGTYPE_PMD defaults to the INFO level, so these messages appear by
default. Customers and tech support use them for debugging and so on.

After the commit:

dev_info() is now enic_pmd_logtype, which defaults to NOTICE. The
macro is still using the INFO level. So, config messages are
suppressed by default. This was never the intention. The current patch
tries to fix that by elevating dev_info to dev_notice, because we do
want these messages to appear by default. Should have done it as part
of the previous commit, but we missed it.

Down the line, we will have to guide our customers to exploit dynamic log
levels, but not this way (i.e. suddenly hiding messages that they used
to see/rely on).

Thanks a lot.
-Hyong

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[Ferruh Yigit]

On 7/26/2019 5:21 AM, Hyong Youb Kim (hyonkim) wrote:
>> -----Original Message-----
>> From: John Daley (johndale)
>> Sent: Friday, July 26, 2019 5:26 AM
>> To: Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Subject: RE: [PATCH] net/enic: retain previous message logging
>>
>> Ok, lets NAK this patch. See comment inline.
>> Thanks,
>> John
>>
> [...]
>>> On 7/25/2019 3:46 AM, John Daley wrote:
>>>> Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
>>>> default. After the fix, using dynamic logging, only NOTICE level and
>>>> higher were displayed by default and INFO level were not. Change the
>>>> messages to NOTICE level so they continue to display.
>>>>
>>>> DTS uses testpmd and parses messages and some tests failed because
>>>> messages were no longer displayed. Other apps may also depend on the
>>>> messages.
>>>
>>> If you need messages for the test framework, why not just increase the log
>>> level for enic PMD via application parameter [1], or as command to
>>> testpmd[2]?
>>> Since it is dynamic debug now, you don't need to change the default, can
>>> change the level on demand.
>>
>> I have no problem modifying our test scripts. The bigger concern was about
>> any other scripts out there that might break because the default enic PMD
>> messages changed. I suppose chances are slim and any such scripts can easily
>> be modified to set the log level to info.
>>
> 
> Hi John, Ferruh,
> 
> Can you guys reconsider? John's commit message makes it sound like he
> is modifying PMD to avoid modifying test scripts. That is not the
> issue at all. The real problem is that his previous commit causes
> a customer visible change, which can lead to a lot of headache for both
> us (doing tech support) and customers (wondering what's changed).
> 
> Prior to commit bbd8ecc05434 ("net/enic: remove PMD log type references"):
> 
> enic prints vNIC config related messages (rq/cq/wq info and such) via
> dev_info(). And, dev_info() uses LOGTYPE_PMD and the INFO level.
> LOGTYPE_PMD defaults to the INFO level, so these messages appear by
> default. Customers and tech support use them for debugging and so on.

I think this is the point, "use them for debugging and so on", if a log is used
for debug should it be always enabled? Specially when log level can be
controlled dynamically in runtime.

If every component selects same path, the dpdk log will be too verbose to be
useful, I think it make sense to log only warnings by default, and if explicitly
set more verbose logging can be enabled for desired component.

And if they are for regular use case (not for debug etc..) the logs are not most
effective way because they require a human parsing them, I believe it worth
thinking if they can be managed in programmatic way, like API return values etc,
instead of logs. And why a user really need a log be default enabled to be able
to run a driver?

> 
> After the commit:
> 
> dev_info() is now enic_pmd_logtype, which defaults to NOTICE. The
> macro is still using the INFO level. So, config messages are
> suppressed by default. This was never the intention. The current patch
> tries to fix that by elevating dev_info to dev_notice, because we do
> want these messages to appear by default. Should have done it as part
> of the previous commit, but we missed it.
> 
> Down the line, we will have to guide our customers to exploit dynamic log
> levels, but not this way (i.e. suddenly hiding messages that they used
> to see/rely on).

I think this is the correct path to follow, DPDK provides API to manage the log
levels dynamically, any user of the DPDK can integrate this into their
application in a way they desire.

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[Stephen Hemminger]

On Fri, 26 Jul 2019 04:21:23 +0000
"Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com> wrote:

> > -----Original Message-----
> > From: John Daley (johndale)
> > Sent: Friday, July 26, 2019 5:26 AM
> > To: Ferruh Yigit <ferruh.yigit@intel.com>
> > Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> > Subject: RE: [PATCH] net/enic: retain previous message logging
> > 
> > Ok, lets NAK this patch. See comment inline.
> > Thanks,
> > John
> >   
> [...]
> > > On 7/25/2019 3:46 AM, John Daley wrote:  
> > > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > > > default. After the fix, using dynamic logging, only NOTICE level and
> > > > higher were displayed by default and INFO level were not. Change the
> > > > messages to NOTICE level so they continue to display.
> > > >
> > > > DTS uses testpmd and parses messages and some tests failed because
> > > > messages were no longer displayed. Other apps may also depend on the
> > > > messages.  
> > >
> > > If you need messages for the test framework, why not just increase the log
> > > level for enic PMD via application parameter [1], or as command to
> > > testpmd[2]?
> > > Since it is dynamic debug now, you don't need to change the default, can
> > > change the level on demand.  
> > 
> > I have no problem modifying our test scripts. The bigger concern was about
> > any other scripts out there that might break because the default enic PMD
> > messages changed. I suppose chances are slim and any such scripts can easily
> > be modified to set the log level to info.
> >   
> 
> Hi John, Ferruh,
> 
> Can you guys reconsider? John's commit message makes it sound like he
> is modifying PMD to avoid modifying test scripts. That is not the
> issue at all. The real problem is that his previous commit causes
> a customer visible change, which can lead to a lot of headache for both
> us (doing tech support) and customers (wondering what's changed).
> 
> Prior to commit bbd8ecc05434 ("net/enic: remove PMD log type references"):
> 
> enic prints vNIC config related messages (rq/cq/wq info and such) via
> dev_info(). And, dev_info() uses LOGTYPE_PMD and the INFO level.
> LOGTYPE_PMD defaults to the INFO level, so these messages appear by
> default. Customers and tech support use them for debugging and so on.
> 
> After the commit:
> 
> dev_info() is now enic_pmd_logtype, which defaults to NOTICE. The
> macro is still using the INFO level. So, config messages are
> suppressed by default. This was never the intention. The current patch
> tries to fix that by elevating dev_info to dev_notice, because we do
> want these messages to appear by default. Should have done it as part
> of the previous commit, but we missed it.
> 
> Down the line, we will have to guide our customers to exploit dynamic log
> levels, but not this way (i.e. suddenly hiding messages that they used
> to see/rely on).
> 
> Thanks a lot.
> -Hyong
> 

Drivers should be silent unless they see a problem.
We don't want every driver outputting messages by default.

For your current issue, why not just register the default log
level as info?

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, July 26, 2019 1:51 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: John Daley (johndale) <johndale@cisco.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging
> 
> On Fri, 26 Jul 2019 04:21:23 +0000
> "Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com> wrote:
> 
> > > -----Original Message-----
> > > From: John Daley (johndale)
> > > Sent: Friday, July 26, 2019 5:26 AM
> > > To: Ferruh Yigit <ferruh.yigit@intel.com>
> > > Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> > > Subject: RE: [PATCH] net/enic: retain previous message logging
> > >
> > > Ok, lets NAK this patch. See comment inline.
> > > Thanks,
> > > John
> > >
> > [...]
> > > > On 7/25/2019 3:46 AM, John Daley wrote:
> > > > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd
> > > > > by default. After the fix, using dynamic logging, only NOTICE
> > > > > level and higher were displayed by default and INFO level were
> > > > > not. Change the messages to NOTICE level so they continue to display.
> > > > >
> > > > > DTS uses testpmd and parses messages and some tests failed
> > > > > because messages were no longer displayed. Other apps may also
> > > > > depend on the messages.
> > > >
> > > > If you need messages for the test framework, why not just increase
> > > > the log level for enic PMD via application parameter [1], or as
> > > > command to testpmd[2]?
> > > > Since it is dynamic debug now, you don't need to change the
> > > > default, can change the level on demand.
> > >
> > > I have no problem modifying our test scripts. The bigger concern was
> > > about any other scripts out there that might break because the
> > > default enic PMD messages changed. I suppose chances are slim and
> > > any such scripts can easily be modified to set the log level to info.
> > >
> >
> > Hi John, Ferruh,
> >
> > Can you guys reconsider? John's commit message makes it sound like he
> > is modifying PMD to avoid modifying test scripts. That is not the
> > issue at all. The real problem is that his previous commit causes a
> > customer visible change, which can lead to a lot of headache for both
> > us (doing tech support) and customers (wondering what's changed).
> >
> > Prior to commit bbd8ecc05434 ("net/enic: remove PMD log type
> references"):
> >
> > enic prints vNIC config related messages (rq/cq/wq info and such) via
> > dev_info(). And, dev_info() uses LOGTYPE_PMD and the INFO level.
> > LOGTYPE_PMD defaults to the INFO level, so these messages appear by
> > default. Customers and tech support use them for debugging and so on.
> >
> > After the commit:
> >
> > dev_info() is now enic_pmd_logtype, which defaults to NOTICE. The
> > macro is still using the INFO level. So, config messages are
> > suppressed by default. This was never the intention. The current patch
> > tries to fix that by elevating dev_info to dev_notice, because we do
> > want these messages to appear by default. Should have done it as part
> > of the previous commit, but we missed it.
> >
> > Down the line, we will have to guide our customers to exploit dynamic
> > log levels, but not this way (i.e. suddenly hiding messages that they
> > used to see/rely on).
> >
> > Thanks a lot.
> > -Hyong
> >
> 
> Drivers should be silent unless they see a problem.
> We don't want every driver outputting messages by default.
> 
> For your current issue, why not just register the default log level as info?

Yes, that is better and what Ferruh suggested also. It is what the v2 patch is: http://patches.dpdk.org/patch/57199/.
Thanks, John

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

Actually, after talking to a couple internal folks, we'd like to get the patch in if possible- many of our customer issues are due to the wrong number of queues, etc, which are reported in the default logs. To ask them to add --log-level=enic,info would be a pain, esp. for apps like OVS, fd.io.
-john

> -----Original Message-----
> From: John Daley (johndale)
> Sent: Thursday, July 25, 2019 1:26 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Subject: RE: [PATCH] net/enic: retain previous message logging
> 
> Ok, lets NAK this patch. See comment inline.
> Thanks,
> John
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Sent: Thursday, July 25, 2019 3:07 AM
> > To: John Daley (johndale) <johndale@cisco.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH] net/enic: retain previous message logging
> >
> > On 7/25/2019 3:46 AM, John Daley wrote:
> > > Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> > > default. After the fix, using dynamic logging, only NOTICE level and
> > > higher were displayed by default and INFO level were not. Change the
> > > messages to NOTICE level so they continue to display.
> > >
> > > DTS uses testpmd and parses messages and some tests failed because
> > > messages were no longer displayed. Other apps may also depend on the
> > > messages.
> >
> > If you need messages for the test framework, why not just increase the
> > log level for enic PMD via application parameter [1], or as command to
> > testpmd[2]?
> > Since it is dynamic debug now, you don't need to change the default,
> > can change the level on demand.
> 
> I have no problem modifying our test scripts. The bigger concern was about
> any other scripts out there that might break because the default enic PMD
> messages changed. I suppose chances are slim and any such scripts can
> easily be modified to set the log level to info.
> 
> >
> > [1]
> > starting testpmd with following option should do it:
> > --log-level=pmd.net.enic.*:info
> >
> > testpmd --log-level=pmd.net.enic.*:info -- -i
> >
> >
> > [2]
> > after testpmd started, can change the debug level:
> > testpmd> set log pmd.net.enic 7
> >
> >
> > [3] bonus, see current log levels
> > testpmd> dump_log_types
> 
> Nice! I didn't know about this.
> 
> >
> >
> >
> > >
> > > Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> > >
> > > Signed-off-by: John Daley <johndale@cisco.com>
> >
> > <...>

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[Ferruh Yigit]

On 7/26/2019 9:17 AM, John Daley (johndale) wrote:
> Actually, after talking to a couple internal folks, we'd like to get the patch in if possible- many of our customer issues are due to the wrong number of queues, etc, which are reported in the default logs. To ask them to add --log-level=enic,info would be a pain, esp. for apps like OVS, fd.io.

As I said to Hyong, I believe it is not good approach to have logs to debug
customer issues enabled by default.

But I see you want to keep the your logging same, instead of replacing all
'dev_info' with 'dev_notice', what about setting default log level for driver to
'RTE_LOG_INFO', this is easier change with same affect?

And when more fine grained update done on which logs to really set to
'dev_notice', the default log level can be updated back to 'RTE_LOG_NOTICE'

> -john
> 
>> -----Original Message-----
>> From: John Daley (johndale)
>> Sent: Thursday, July 25, 2019 1:26 PM
>> To: Ferruh Yigit <ferruh.yigit@intel.com>
>> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
>> Subject: RE: [PATCH] net/enic: retain previous message logging
>>
>> Ok, lets NAK this patch. See comment inline.
>> Thanks,
>> John
>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Thursday, July 25, 2019 3:07 AM
>>> To: John Daley (johndale) <johndale@cisco.com>
>>> Cc: dev@dpdk.org
>>> Subject: Re: [PATCH] net/enic: retain previous message logging
>>>
>>> On 7/25/2019 3:46 AM, John Daley wrote:
>>>> Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
>>>> default. After the fix, using dynamic logging, only NOTICE level and
>>>> higher were displayed by default and INFO level were not. Change the
>>>> messages to NOTICE level so they continue to display.
>>>>
>>>> DTS uses testpmd and parses messages and some tests failed because
>>>> messages were no longer displayed. Other apps may also depend on the
>>>> messages.
>>>
>>> If you need messages for the test framework, why not just increase the
>>> log level for enic PMD via application parameter [1], or as command to
>>> testpmd[2]?
>>> Since it is dynamic debug now, you don't need to change the default,
>>> can change the level on demand.
>>
>> I have no problem modifying our test scripts. The bigger concern was about
>> any other scripts out there that might break because the default enic PMD
>> messages changed. I suppose chances are slim and any such scripts can
>> easily be modified to set the log level to info.
>>
>>>
>>> [1]
>>> starting testpmd with following option should do it:
>>> --log-level=pmd.net.enic.*:info
>>>
>>> testpmd --log-level=pmd.net.enic.*:info -- -i
>>>
>>>
>>> [2]
>>> after testpmd started, can change the debug level:
>>> testpmd> set log pmd.net.enic 7
>>>
>>>
>>> [3] bonus, see current log levels
>>> testpmd> dump_log_types
>>
>> Nice! I didn't know about this.
>>
>>>
>>>
>>>
>>>>
>>>> Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
>>>>
>>>> Signed-off-by: John Daley <johndale@cisco.com>
>>>
>>> <...>
> 

Re: [dpdk-dev] [PATCH] net/enic: retain previous message logging

[John Daley (johndale)]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, July 26, 2019 3:01 AM
> To: John Daley (johndale) <johndale@cisco.com>
> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Subject: Re: [PATCH] net/enic: retain previous message logging
> 
> On 7/26/2019 9:17 AM, John Daley (johndale) wrote:
> > Actually, after talking to a couple internal folks, we'd like to get the patch in
> if possible- many of our customer issues are due to the wrong number of
> queues, etc, which are reported in the default logs. To ask them to add --log-
> level=enic,info would be a pain, esp. for apps like OVS, fd.io.
> 
> As I said to Hyong, I believe it is not good approach to have logs to debug
> customer issues enabled by default.

Yes, we need to migrate away from this, but to suddenly hide messages that have been there all along is worse. We definitely need to work towards less verbose default messaging.
> 
> But I see you want to keep the your logging same, instead of replacing all
> 'dev_info' with 'dev_notice', what about setting default log level for driver to
> 'RTE_LOG_INFO', this is easier change with same affect?
> 
> And when more fine grained update done on which logs to really set to
> 'dev_notice', the default log level can be updated back to 'RTE_LOG_NOTICE'

I agree this is a better way to go. I have a V2 patch coming which sets the enic PMD default log level to RTE_LOG_INFO. We can submit fine grained updates in future commits.
Thanks, John.
>
> > -john
> >
> >> -----Original Message-----
> >> From: John Daley (johndale)
> >> Sent: Thursday, July 25, 2019 1:26 PM
> >> To: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Cc: dev@dpdk.org; Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> >> Subject: RE: [PATCH] net/enic: retain previous message logging
> >>
> >> Ok, lets NAK this patch. See comment inline.
> >> Thanks,
> >> John
> >>
> >>> -----Original Message-----
> >>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> Sent: Thursday, July 25, 2019 3:07 AM
> >>> To: John Daley (johndale) <johndale@cisco.com>
> >>> Cc: dev@dpdk.org
> >>> Subject: Re: [PATCH] net/enic: retain previous message logging
> >>>
> >>> On 7/25/2019 3:46 AM, John Daley wrote:
> >>>> Prior to fix, RTE_LOGTYPE_INFO messages would display in testpmd by
> >>>> default. After the fix, using dynamic logging, only NOTICE level
> >>>> and higher were displayed by default and INFO level were not.
> >>>> Change the messages to NOTICE level so they continue to display.
> >>>>
> >>>> DTS uses testpmd and parses messages and some tests failed because
> >>>> messages were no longer displayed. Other apps may also depend on
> >>>> the messages.
> >>>
> >>> If you need messages for the test framework, why not just increase
> >>> the log level for enic PMD via application parameter [1], or as
> >>> command to testpmd[2]?
> >>> Since it is dynamic debug now, you don't need to change the default,
> >>> can change the level on demand.
> >>
> >> I have no problem modifying our test scripts. The bigger concern was
> >> about any other scripts out there that might break because the
> >> default enic PMD messages changed. I suppose chances are slim and any
> >> such scripts can easily be modified to set the log level to info.
> >>
> >>>
> >>> [1]
> >>> starting testpmd with following option should do it:
> >>> --log-level=pmd.net.enic.*:info
> >>>
> >>> testpmd --log-level=pmd.net.enic.*:info -- -i
> >>>
> >>>
> >>> [2]
> >>> after testpmd started, can change the debug level:
> >>> testpmd> set log pmd.net.enic 7
> >>>
> >>>
> >>> [3] bonus, see current log levels
> >>> testpmd> dump_log_types
> >>
> >> Nice! I didn't know about this.
> >>
> >>>
> >>>
> >>>
> >>>>
> >>>> Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> >>>>
> >>>> Signed-off-by: John Daley <johndale@cisco.com>
> >>>
> >>> <...>
> >

[dpdk-dev] [PATCH v2] net/enic: retain previous message logging

[John Daley]

Prior to the fix, RTE_LOGTYPE_INFO messages were displayed by default.
After the fix, only NOTICE level and higher were displayed by default
and INFO level were not. There are INFO level vNIC config related
messages which customers and tech support currently depend on for
debugging and so on and to suddenly hide these messages is not a good
idea.

This patch changes the default log level to RTE_LOG_INFO for enic so
messages are printed as before the fix.

Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")

Signed-off-by: John Daley <johndale@cisco.com>
---

v2: Change default log level to 'RTE_LOG_INFO' instead of changing the
    'dev_info' messages to 'dev_notice'

 drivers/net/enic/enic_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
index 6051689a6..0ade57370 100644
--- a/drivers/net/enic/enic_ethdev.c
+++ b/drivers/net/enic/enic_ethdev.c
@@ -73,7 +73,7 @@ RTE_INIT(enicpmd_init_log)
 {
 	enic_pmd_logtype = rte_log_register("pmd.net.enic");
 	if (enic_pmd_logtype >= 0)
-		rte_log_set_level(enic_pmd_logtype, RTE_LOG_NOTICE);
+		rte_log_set_level(enic_pmd_logtype, RTE_LOG_INFO);
 }
 
 static int
-- 
2.22.0

Re: [dpdk-dev] [PATCH v2] net/enic: retain previous message logging

[Ferruh Yigit]

On 7/26/2019 9:12 PM, John Daley wrote:
> Prior to the fix, RTE_LOGTYPE_INFO messages were displayed by default.
> After the fix, only NOTICE level and higher were displayed by default
> and INFO level were not. There are INFO level vNIC config related
> messages which customers and tech support currently depend on for
> debugging and so on and to suddenly hide these messages is not a good
> idea.
> 
> This patch changes the default log level to RTE_LOG_INFO for enic so
> messages are printed as before the fix.
> 
> Fixes: bbd8ecc05434 ("net/enic: remove PMD log type references")
> 
> Signed-off-by: John Daley <johndale@cisco.com>

Applied to dpdk-next-net/master, thanks.