From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <ktraynor@redhat.com>
Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73])
by dpdk.org (Postfix) with ESMTP id 65B3D1B4C7
for <dev@dpdk.org>; Thu, 2 Aug 2018 16:09:55 +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 B14DD818B107;
Thu, 2 Aug 2018 14:09:54 +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 054AB1101E87;
Thu, 2 Aug 2018 14:09:53 +0000 (UTC)
To: Andrew Rybchenko <arybchenko@solarflare.com>, dev@dpdk.org
References: <1533213223-24028-1-git-send-email-ktraynor@redhat.com>
<661cb701-42a8-1416-1e78-8fa316f0300a@solarflare.com>
<d94cce34-1335-1372-8f25-34e79ab935e2@redhat.com>
<7f50a75f-d585-d8fd-5b31-783a5c4a37bc@solarflare.com>
From: Kevin Traynor <ktraynor@redhat.com>
Organization: Red Hat
Message-ID: <9c85e414-f5d3-2450-7ddb-c954ce737c31@redhat.com>
Date: Thu, 2 Aug 2018 15:09:52 +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: <7f50a75f-d585-d8fd-5b31-783a5c4a37bc@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.8]); Thu, 02 Aug 2018 14:09:54 +0000 (UTC)
X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]);
Thu, 02 Aug 2018 14:09:54 +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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 02 Aug 2018 14:09:55 -0000
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 <ktraynor@redhat.com>
>>>> ---
>>>> 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.
* @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.
>