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

  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(-)
  

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;
>   	}
>
  

> -----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;
> >   	}
> >
  

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;
>>>    	}
>>>
  

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;
> >>>    	}
> >>>
  

> Can this serie below be applied now?
> Thanks and regards,
> Nic
Series Applied to dpd-next-crypto

Thanks.