From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 2C15D1B4CE for ; Thu, 2 Aug 2018 16:52:48 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7E4D5F2B34; Thu, 2 Aug 2018 14:52:47 +0000 (UTC) Received: from ktraynor.remote.csb (ovpn-117-113.ams2.redhat.com [10.36.117.113]) by smtp.corp.redhat.com (Postfix) with ESMTP id 437F21007084; Thu, 2 Aug 2018 14:52:45 +0000 (UTC) To: Andrew Rybchenko , dev@dpdk.org Cc: Ferruh Yigit , Thomas Monjalon References: <1533213223-24028-1-git-send-email-ktraynor@redhat.com> <661cb701-42a8-1416-1e78-8fa316f0300a@solarflare.com> <7f50a75f-d585-d8fd-5b31-783a5c4a37bc@solarflare.com> <9c85e414-f5d3-2450-7ddb-c954ce737c31@redhat.com> <00c35ad3-e063-b14e-ef56-e4308ec83f52@solarflare.com> From: Kevin Traynor Organization: Red Hat Message-ID: <581602cf-36ba-2d6f-03ab-b00e9817f2a8@redhat.com> Date: Thu, 2 Aug 2018 15:52:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <00c35ad3-e063-b14e-ef56-e4308ec83f52@solarflare.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 02 Aug 2018 14:52:47 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.2]); Thu, 02 Aug 2018 14:52:47 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'ktraynor@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Aug 2018 14:52:48 -0000 On 08/02/2018 03:41 PM, Andrew Rybchenko wrote: > On 02.08.2018 17:09, Kevin Traynor wrote: >> On 08/02/2018 02:52 PM, Andrew Rybchenko wrote: >>> On 02.08.2018 16:35, Kevin Traynor wrote: >>>> On 08/02/2018 01:59 PM, Andrew Rybchenko wrote: >>>>> On 02.08.2018 15:33, Kevin Traynor wrote: >>>>>> Change log level of messages from ERR back to DEBUG where >>>>>> existing API indicates that the condition is a success. >>>>>> >>>>>> This means applications using the API in its current form will >>>>>> not get new ERR logs. >>>>>> >>>>>> Fixes: bea1e0c70cfc ("ethdev: convert static log type usage to >>>>>> dynamic") >>>>>> >>>>>> Signed-off-by: Kevin Traynor >>>>>> --- >>>>>> lib/librte_ethdev/rte_ethdev.c | 12 ++++++------ >>>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c >>>>>> b/lib/librte_ethdev/rte_ethdev.c >>>>>> index c7ab157..16656d2 100644 >>>>>> --- a/lib/librte_ethdev/rte_ethdev.c >>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c >>>>>> @@ -797,5 +797,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->rx_queue_state[rx_queue_id] != >>>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>>> already started\n", >>>>>> rx_queue_id, port_id); >>>>>> @@ -824,5 +824,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->rx_queue_state[rx_queue_id] == >>>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>>> already stopped\n", >>>>>> rx_queue_id, port_id); >>>>>> @@ -857,5 +857,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->tx_queue_state[tx_queue_id] != >>>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>>> already started\n", >>>>>> tx_queue_id, port_id); >>>>>> @@ -882,5 +882,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->tx_queue_state[tx_queue_id] == >>>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>>> already stopped\n", >>>>>> tx_queue_id, port_id); >>>>>> @@ -1287,5 +1287,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->dev_started != 0) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Device with port_id=%"PRIu16" already started\n", >>>>>> port_id); >>>>>> @@ -1319,5 +1319,5 @@ struct rte_eth_dev * >>>>>> if (dev->data->dev_started == 0) { >>>>>> - RTE_ETHDEV_LOG(ERR, >>>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>>> "Device with port_id=%"PRIu16" already stopped\n", >>>>>> port_id); >>>>> I would suggest to use WARNING here. Yes, it is not an error since >>>>> nothing bad has >>>>> happened and we handle duplicate stop and duplicate start, >>>>> but I think it is bad that (buggy?) application does it. Making it >>>>> debug >>>>> we simply >>>>> hide it too much. >>>>> >>>> I think an application following the documented API is not bad or >>>> buggy. >>> I've failed to find the place were it is documented that device/queue >>> may be stopped/started twice. Could you point out? >>> Yes, return value 0 means success, but it is a separate thing. >>> >> I was commenting directly on the API and it's documentation e.g. below >> for dev start. I'm not aware of other documentation specifying how it >> can/cannot be called. > > I would not say so. "0: Success. Ethernet device started" means that > function managed to do the job and finally the device is started. > > Never-mind it is not that important and already paid to much attention. > I've included in CC other ethdev maintainers (who should be there from > the very beginning). I don't mind if it is acked by other ethdev maintainer > and applied. It is definitely not an error as it is now. Thanks. > Sure, sounds good - I'm glad we agree on something :-) I'll leave it for a day and can re-spin tomorrow. thanks, Kevin. >> * @return >> * - 0: Success, Ethernet device started. >> * - <0: Error code of the driver device start function. >> */ >> int rte_eth_dev_start(uint16_t port_id); >> >> int >> rte_eth_dev_start(uint16_t port_id) >> { >> ... >> if (dev->data->dev_started != 0) { >> RTE_ETHDEV_LOG(DEBUG, >> "Device with port_id=%"PRIu16" already started\n", >> port_id); >> return 0; >> } >> ... >> } >> >> >>>> An application may not maintain some state and always call a stop >>>> before >>>> a start etc. in line with the API. >>>> >>>> I don't think an API success condition should be changed from a debug >>>> message to anything else. Otherwise, it is trying to flag a warning >>>> about the application which is following the API! It and will just >>>> spook >>>> people when they see a new warning. >