* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 12:33 [dpdk-dev] [PATCH] ethdev: decrease log level for successful API Kevin Traynor
@ 2018-08-02 12:59 ` Andrew Rybchenko
2018-08-02 13:35 ` Kevin Traynor
2018-08-02 12:59 ` Andrew Rybchenko
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-02 12:59 UTC (permalink / raw)
To: Kevin Traynor, dev
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 12:59 ` Andrew Rybchenko
@ 2018-08-02 13:35 ` Kevin Traynor
2018-08-02 13:52 ` Andrew Rybchenko
0 siblings, 1 reply; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 13:35 UTC (permalink / raw)
To: Andrew Rybchenko, dev
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.
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 13:35 ` Kevin Traynor
@ 2018-08-02 13:52 ` Andrew Rybchenko
2018-08-02 14:09 ` Kevin Traynor
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-02 13:52 UTC (permalink / raw)
To: Kevin Traynor, dev
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.
> 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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 13:52 ` Andrew Rybchenko
@ 2018-08-02 14:09 ` Kevin Traynor
2018-08-02 14:41 ` Andrew Rybchenko
0 siblings, 1 reply; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 14:09 UTC (permalink / raw)
To: Andrew Rybchenko, dev
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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 14:09 ` Kevin Traynor
@ 2018-08-02 14:41 ` Andrew Rybchenko
2018-08-02 14:52 ` Kevin Traynor
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-02 14:41 UTC (permalink / raw)
To: Kevin Traynor, dev; +Cc: Ferruh Yigit, Thomas Monjalon
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 <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.
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.
> * @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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 14:41 ` Andrew Rybchenko
@ 2018-08-02 14:52 ` Kevin Traynor
2018-08-02 16:14 ` Thomas Monjalon
0 siblings, 1 reply; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 14:52 UTC (permalink / raw)
To: Andrew Rybchenko, dev; +Cc: Ferruh Yigit, Thomas Monjalon
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 <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.
>
> 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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 14:52 ` Kevin Traynor
@ 2018-08-02 16:14 ` Thomas Monjalon
2018-08-02 18:01 ` Andrew Rybchenko
0 siblings, 1 reply; 17+ messages in thread
From: Thomas Monjalon @ 2018-08-02 16:14 UTC (permalink / raw)
To: Kevin Traynor, Andrew Rybchenko; +Cc: dev, Ferruh Yigit
02/08/2018 16:52, Kevin Traynor:
> 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:
> >>>>>> @@ -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.
I would like to give an opinion, but unfortunately it's hard to decide.
For sure, it should not be a WARNING level (used for non-fatal errors).
What else do we have? NOTICE, INFO and DEBUG.
I think it can be INFO.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 16:14 ` Thomas Monjalon
@ 2018-08-02 18:01 ` Andrew Rybchenko
2018-08-02 18:27 ` Kevin Traynor
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-02 18:01 UTC (permalink / raw)
To: Thomas Monjalon, Kevin Traynor; +Cc: dev, Ferruh Yigit, Stephen Hemminger
On 02.08.2018 19:14, Thomas Monjalon wrote:
> 02/08/2018 16:52, Kevin Traynor:
>> 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:
>>>>>>>> @@ -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.
> I would like to give an opinion, but unfortunately it's hard to decide.
> For sure, it should not be a WARNING level (used for non-fatal errors).
> What else do we have? NOTICE, INFO and DEBUG.
> I think it can be INFO.
So, we have ERR (now), WARNING (my initial suggestion), NOTICE
(by Stephen), INFO (by Thomas) and DEBUG (by Kevin) :)
If it is not treated as an error in application behaviour, I agree that my
suggestion of WARNING is not suitable. Typically NOTICE is the default
log level and if we consider such behaviour of apps correct, it should
be INFO to be silent by default. I really don't like DEBUG since these
messages are still important and could be simply lost in DEBUG
which could to be very-very verbose.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 18:01 ` Andrew Rybchenko
@ 2018-08-02 18:27 ` Kevin Traynor
0 siblings, 0 replies; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 18:27 UTC (permalink / raw)
To: Andrew Rybchenko, Thomas Monjalon; +Cc: dev, Ferruh Yigit, Stephen Hemminger
On 08/02/2018 07:01 PM, Andrew Rybchenko wrote:
> On 02.08.2018 19:14, Thomas Monjalon wrote:
>> 02/08/2018 16:52, Kevin Traynor:
>>> 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:
>>>>>>>>> @@ -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.
>> I would like to give an opinion, but unfortunately it's hard to decide.
>> For sure, it should not be a WARNING level (used for non-fatal errors).
>> What else do we have? NOTICE, INFO and DEBUG.
>> I think it can be INFO.
>
> So, we have ERR (now), WARNING (my initial suggestion), NOTICE
> (by Stephen), INFO (by Thomas) and DEBUG (by Kevin) :)
>
Well, we completed the set :-)
> If it is not treated as an error in application behaviour, I agree that my
> suggestion of WARNING is not suitable. Typically NOTICE is the default
> log level and if we consider such behaviour of apps correct, it should
> be INFO to be silent by default. I really don't like DEBUG since these
> messages are still important and could be simply lost in DEBUG
> which could to be very-very verbose.
ok, I'll update and send as INFO before someone suggests adding new log
levels to choose from.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 12:33 [dpdk-dev] [PATCH] ethdev: decrease log level for successful API Kevin Traynor
2018-08-02 12:59 ` Andrew Rybchenko
@ 2018-08-02 12:59 ` Andrew Rybchenko
2018-08-02 15:14 ` Stephen Hemminger
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-02 12:59 UTC (permalink / raw)
To: Kevin Traynor, dev
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 12:33 [dpdk-dev] [PATCH] ethdev: decrease log level for successful API Kevin Traynor
2018-08-02 12:59 ` Andrew Rybchenko
2018-08-02 12:59 ` Andrew Rybchenko
@ 2018-08-02 15:14 ` Stephen Hemminger
2018-08-02 18:33 ` Kevin Traynor
2018-08-02 18:39 ` [dpdk-dev] [PATCH v2] " Kevin Traynor
4 siblings, 0 replies; 17+ messages in thread
From: Stephen Hemminger @ 2018-08-02 15:14 UTC (permalink / raw)
To: Kevin Traynor; +Cc: dev
On Thu, 2 Aug 2018 13:33:43 +0100
Kevin Traynor <ktraynor@redhat.com> 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")
The expectations about level come from syslog:
LOG_ERR error conditions
LOG_WARNING warning conditions
LOG_NOTICE normal, but significant, condition
LOG_INFO informational message
LOG_DEBUG debug-level message
Therefore messages where an API did something wrong should be NOTICE
and messages where an API's normal action was successful should be
INFO or DEBUG.
All the "already XXX" messages should be NOTICE.
^ permalink raw reply [flat|nested] 17+ messages in thread* [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 12:33 [dpdk-dev] [PATCH] ethdev: decrease log level for successful API Kevin Traynor
` (2 preceding siblings ...)
2018-08-02 15:14 ` Stephen Hemminger
@ 2018-08-02 18:33 ` Kevin Traynor
2018-08-02 18:39 ` Kevin Traynor
2018-08-02 18:39 ` [dpdk-dev] [PATCH v2] " Kevin Traynor
4 siblings, 1 reply; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 18:33 UTC (permalink / raw)
To: dev; +Cc: Kevin Traynor
Change log level of messages from ERR to INFO where
the post condition of the API is success, but no action
was actually needed as the condition already existed.
e.g. calling rte_eth_dev_start() for a device that is
already started.
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..4c32025 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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"Device with port_id=%"PRIu16" already stopped\n",
port_id);
--
1.8.3.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API
2018-08-02 18:33 ` Kevin Traynor
@ 2018-08-02 18:39 ` Kevin Traynor
0 siblings, 0 replies; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 18:39 UTC (permalink / raw)
To: Kevin Traynor, dev
subject prefix not updated - will resend as [PATCH v2]
On 08/02/2018 07:33 PM, Kevin Traynor wrote:
> Change log level of messages from ERR to INFO where
> the post condition of the API is success, but no action
> was actually needed as the condition already existed.
> e.g. calling rte_eth_dev_start() for a device that is
> already started.
>
> 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..4c32025 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(INFO,
> "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(INFO,
> "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(INFO,
> "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(INFO,
> "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(INFO,
> "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(INFO,
> "Device with port_id=%"PRIu16" already stopped\n",
> port_id);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [dpdk-dev] [PATCH v2] ethdev: decrease log level for successful API
2018-08-02 12:33 [dpdk-dev] [PATCH] ethdev: decrease log level for successful API Kevin Traynor
` (3 preceding siblings ...)
2018-08-02 18:33 ` Kevin Traynor
@ 2018-08-02 18:39 ` Kevin Traynor
2018-08-03 8:04 ` Andrew Rybchenko
4 siblings, 1 reply; 17+ messages in thread
From: Kevin Traynor @ 2018-08-02 18:39 UTC (permalink / raw)
To: dev; +Cc: Kevin Traynor
Change log level of messages from ERR to INFO where
the post condition of the API is success, but no action
was actually needed as the condition already existed.
e.g. calling rte_eth_dev_start() for a device that is
already started.
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..4c32025 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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"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(INFO,
"Device with port_id=%"PRIu16" already stopped\n",
port_id);
--
1.8.3.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: decrease log level for successful API
2018-08-02 18:39 ` [dpdk-dev] [PATCH v2] " Kevin Traynor
@ 2018-08-03 8:04 ` Andrew Rybchenko
2018-08-05 13:29 ` Thomas Monjalon
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Rybchenko @ 2018-08-03 8:04 UTC (permalink / raw)
To: Kevin Traynor, dev
On 02.08.2018 21:39, Kevin Traynor wrote:
> Change log level of messages from ERR to INFO where
> the post condition of the API is success, but no action
> was actually needed as the condition already existed.
> e.g. calling rte_eth_dev_start() for a device that is
> already started.
>
> Fixes: bea1e0c70cfc ("ethdev: convert static log type usage to dynamic")
>
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [dpdk-dev] [PATCH v2] ethdev: decrease log level for successful API
2018-08-03 8:04 ` Andrew Rybchenko
@ 2018-08-05 13:29 ` Thomas Monjalon
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2018-08-05 13:29 UTC (permalink / raw)
To: Kevin Traynor; +Cc: dev, Andrew Rybchenko
03/08/2018 10:04, Andrew Rybchenko:
> On 02.08.2018 21:39, Kevin Traynor wrote:
> > Change log level of messages from ERR to INFO where
> > the post condition of the API is success, but no action
> > was actually needed as the condition already existed.
> > e.g. calling rte_eth_dev_start() for a device that is
> > already started.
> >
> > Fixes: bea1e0c70cfc ("ethdev: convert static log type usage to dynamic")
> >
> > Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
Applied, thanks
^ permalink raw reply [flat|nested] 17+ messages in thread