diff mbox series

[v3,6/6] bbdev: reduce warning level for one scenario

Message ID 1631063741-28631-7-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Accepted, archived
Delegated to: akhil goyal
Headers show
Series bbdev update related to CRC usage | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Chautru, Nicolas Sept. 8, 2021, 1:15 a.m. UTC
Queue setup may genuinely fail when adding incremental queues
for a given priority level. In that case application would
attempt to configure a queue at a different priority level.
Not an actual error.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 lib/bbdev/rte_bbdev.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Tom Rix Sept. 12, 2021, 12:54 p.m. UTC | #1
On 9/7/21 6:15 PM, Nicolas Chautru wrote:
> Queue setup may genuinely fail when adding incremental queues
> for a given priority level. In that case application would
> attempt to configure a queue at a different priority level.
> Not an actual error.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   lib/bbdev/rte_bbdev.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
> index fc37236..defddcf 100644
> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -528,9 +528,10 @@ struct rte_bbdev *
>   	ret = dev->dev_ops->queue_setup(dev, queue_id, (conf != NULL) ?
>   			conf : &dev_info.default_queue_conf);
>   	if (ret < 0) {
> -		rte_bbdev_log(ERR,
> -				"Device %u queue %u setup failed", dev_id,
> -				queue_id);
> +		/* This may happen when trying different priority levels */
> +		rte_bbdev_log(INFO,
> +				"Device %u queue %u setup failed",
> +				dev_id, queue_id);

This change is just changing the log level, which is fine.

I am looking at how the error handling is done for the function.

It seems like the bailing is done in the middle of change the queue state.

ex/ the block above this one

/* Release existing queue ... */

Does this leave the queue in a bad state ?

Tom

>   		return ret;
>   	}
>
Chautru, Nicolas Sept. 13, 2021, 5:03 p.m. UTC | #2
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Sunday, September 12, 2021 5:55 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> gakhil@marvell.com
> Cc: thomas@monjalon.net; hemant.agrawal@nxp.com; Zhang, Mingshan
> <mingshan.zhang@intel.com>; Joshi, Arun <arun.joshi@intel.com>
> Subject: Re: [PATCH v3 6/6] bbdev: reduce warning level for one scenario
> 
> 
> On 9/7/21 6:15 PM, Nicolas Chautru wrote:
> > Queue setup may genuinely fail when adding incremental queues for a
> > given priority level. In that case application would attempt to
> > configure a queue at a different priority level.
> > Not an actual error.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   lib/bbdev/rte_bbdev.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> > fc37236..defddcf 100644
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -528,9 +528,10 @@ struct rte_bbdev *
> >   	ret = dev->dev_ops->queue_setup(dev, queue_id, (conf != NULL) ?
> >   			conf : &dev_info.default_queue_conf);
> >   	if (ret < 0) {
> > -		rte_bbdev_log(ERR,
> > -				"Device %u queue %u setup failed", dev_id,
> > -				queue_id);
> > +		/* This may happen when trying different priority levels */
> > +		rte_bbdev_log(INFO,
> > +				"Device %u queue %u setup failed",
> > +				dev_id, queue_id);
> 
> This change is just changing the log level, which is fine.
> 
> I am looking at how the error handling is done for the function.
> 
> It seems like the bailing is done in the middle of change the queue state.
> 
> ex/ the block above this one
> 
> /* Release existing queue ... */
> 
> Does this leave the queue in a bad state ?

Hi Tom, 
That would not be related to that change indeed. 

The queue would end up in a not configured when rte_bbdev_queue_configure() fails but then can still  be configured again without limitation (worst thing than can happen is that queue_release is called, hence leaves the queue in a deterministic state, unconfigured but ready to be configured).
Note that queue_release() just removes the configuration of the queue, but the queue is still there and can be configured again (actual total number of queues unchanged, based on number previously set with rte_bbdev_setup_queues()).

Thanks
Nic

> 
> Tom
> 
> >   		return ret;
> >   	}
> >
Tom Rix Sept. 13, 2021, 8:01 p.m. UTC | #3
On 9/13/21 10:03 AM, Chautru, Nicolas wrote:
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Sunday, September 12, 2021 5:55 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com
>> Cc: thomas@monjalon.net; hemant.agrawal@nxp.com; Zhang, Mingshan
>> <mingshan.zhang@intel.com>; Joshi, Arun <arun.joshi@intel.com>
>> Subject: Re: [PATCH v3 6/6] bbdev: reduce warning level for one scenario
>>
>>
>> On 9/7/21 6:15 PM, Nicolas Chautru wrote:
>>> Queue setup may genuinely fail when adding incremental queues for a
>>> given priority level. In that case application would attempt to
>>> configure a queue at a different priority level.
>>> Not an actual error.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    lib/bbdev/rte_bbdev.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
>>> fc37236..defddcf 100644
>>> --- a/lib/bbdev/rte_bbdev.c
>>> +++ b/lib/bbdev/rte_bbdev.c
>>> @@ -528,9 +528,10 @@ struct rte_bbdev *
>>>    	ret = dev->dev_ops->queue_setup(dev, queue_id, (conf != NULL) ?
>>>    			conf : &dev_info.default_queue_conf);
>>>    	if (ret < 0) {
>>> -		rte_bbdev_log(ERR,
>>> -				"Device %u queue %u setup failed", dev_id,
>>> -				queue_id);
>>> +		/* This may happen when trying different priority levels */
>>> +		rte_bbdev_log(INFO,
>>> +				"Device %u queue %u setup failed",
>>> +				dev_id, queue_id);
>> This change is just changing the log level, which is fine.
>>
>> I am looking at how the error handling is done for the function.
>>
>> It seems like the bailing is done in the middle of change the queue state.
>>
>> ex/ the block above this one
>>
>> /* Release existing queue ... */
>>
>> Does this leave the queue in a bad state ?
> Hi Tom,
> That would not be related to that change indeed.
>
> The queue would end up in a not configured when rte_bbdev_queue_configure() fails but then can still  be configured again without limitation (worst thing than can happen is that queue_release is called, hence leaves the queue in a deterministic state, unconfigured but ready to be configured).
> Note that queue_release() just removes the configuration of the queue, but the queue is still there and can be configured again (actual total number of queues unchanged, based on number previously set with rte_bbdev_setup_queues()).

So its in a bad state, but outside the scope of this commit.

Reviewed-by: Tom Rix <trix@redhat.com>

Tom

>
> Thanks
> Nic
>
>> Tom
>>
>>>    		return ret;
>>>    	}
>>>
Chautru, Nicolas Oct. 4, 2021, 11:40 p.m. UTC | #4
Hi Akhil, 
Can this serie below be applied now?
Thanks and regards,
Nic

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Monday, September 13, 2021 1:02 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> gakhil@marvell.com
> Cc: thomas@monjalon.net; hemant.agrawal@nxp.com; Zhang, Mingshan
> <mingshan.zhang@intel.com>; Joshi, Arun <arun.joshi@intel.com>
> Subject: Re: [PATCH v3 6/6] bbdev: reduce warning level for one scenario
> 
> 
> On 9/13/21 10:03 AM, Chautru, Nicolas wrote:
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Sent: Sunday, September 12, 2021 5:55 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> gakhil@marvell.com
> >> Cc: thomas@monjalon.net; hemant.agrawal@nxp.com; Zhang, Mingshan
> >> <mingshan.zhang@intel.com>; Joshi, Arun <arun.joshi@intel.com>
> >> Subject: Re: [PATCH v3 6/6] bbdev: reduce warning level for one
> >> scenario
> >>
> >>
> >> On 9/7/21 6:15 PM, Nicolas Chautru wrote:
> >>> Queue setup may genuinely fail when adding incremental queues for a
> >>> given priority level. In that case application would attempt to
> >>> configure a queue at a different priority level.
> >>> Not an actual error.
> >>>
> >>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >>> ---
> >>>    lib/bbdev/rte_bbdev.c | 7 ++++---
> >>>    1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> >>> fc37236..defddcf 100644
> >>> --- a/lib/bbdev/rte_bbdev.c
> >>> +++ b/lib/bbdev/rte_bbdev.c
> >>> @@ -528,9 +528,10 @@ struct rte_bbdev *
> >>>    	ret = dev->dev_ops->queue_setup(dev, queue_id, (conf != NULL) ?
> >>>    			conf : &dev_info.default_queue_conf);
> >>>    	if (ret < 0) {
> >>> -		rte_bbdev_log(ERR,
> >>> -				"Device %u queue %u setup failed", dev_id,
> >>> -				queue_id);
> >>> +		/* This may happen when trying different priority levels */
> >>> +		rte_bbdev_log(INFO,
> >>> +				"Device %u queue %u setup failed",
> >>> +				dev_id, queue_id);
> >> This change is just changing the log level, which is fine.
> >>
> >> I am looking at how the error handling is done for the function.
> >>
> >> It seems like the bailing is done in the middle of change the queue state.
> >>
> >> ex/ the block above this one
> >>
> >> /* Release existing queue ... */
> >>
> >> Does this leave the queue in a bad state ?
> > Hi Tom,
> > That would not be related to that change indeed.
> >
> > The queue would end up in a not configured when
> rte_bbdev_queue_configure() fails but then can still  be configured again
> without limitation (worst thing than can happen is that queue_release is
> called, hence leaves the queue in a deterministic state, unconfigured but
> ready to be configured).
> > Note that queue_release() just removes the configuration of the queue,
> but the queue is still there and can be configured again (actual total number
> of queues unchanged, based on number previously set with
> rte_bbdev_setup_queues()).
> 
> So its in a bad state, but outside the scope of this commit.
> 
> Reviewed-by: Tom Rix <trix@redhat.com>
> 
> Tom
> 
> >
> > Thanks
> > Nic
> >
> >> Tom
> >>
> >>>    		return ret;
> >>>    	}
> >>>
Akhil Goyal Oct. 5, 2021, 10:05 a.m. UTC | #5
> Can this serie below be applied now?
> Thanks and regards,
> Nic
Series Applied to dpd-next-crypto

Thanks.
diff mbox series

Patch

diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
index fc37236..defddcf 100644
--- a/lib/bbdev/rte_bbdev.c
+++ b/lib/bbdev/rte_bbdev.c
@@ -528,9 +528,10 @@  struct rte_bbdev *
 	ret = dev->dev_ops->queue_setup(dev, queue_id, (conf != NULL) ?
 			conf : &dev_info.default_queue_conf);
 	if (ret < 0) {
-		rte_bbdev_log(ERR,
-				"Device %u queue %u setup failed", dev_id,
-				queue_id);
+		/* This may happen when trying different priority levels */
+		rte_bbdev_log(INFO,
+				"Device %u queue %u setup failed",
+				dev_id, queue_id);
 		return ret;
 	}