Re: [dpdk-dev] Bulk dequeue of packets and the returned values, question

["Wiles, Roger Keith" <keith.wiles@windriver.com>]

On Sep 29, 2014, at 7:30 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:

> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>> Sent: Monday, September 29, 2014 1:10 PM
>> To: Wiles, Roger Keith (Wind River)
>> Cc: dev@dpdk.org
>> Subject: Re: [dpdk-dev] Bulk dequeue of packets and the returned values, question
>> 
>> On Sun, Sep 28, 2014 at 11:06:17PM +0000, Wiles, Roger Keith wrote:
>>> Thanks Venky,
>>> On Sep 28, 2014, at 5:23 PM, Venkatesan, Venky <venky.venkatesan@intel.com> wrote:
>>> 
>>>> Keith,
>>>> 
>>>> On 9/28/2014 11:04 AM, Wiles, Roger Keith wrote:
>>>>> I am also looking at the bulk dequeue routines, which the ring can be fixed or variable. On fixed  < 0 on error is returned and 0 if
>> successful. On a variable ring < 0 on error or n on success, but I think n can be zero in the variable case, correct?
>>>>> 
>>>>> If these are true then why not have the routines return  < 0 on error and >= 0 on success. Which means a dequeue from a fixed
>> ring would return only ’requested size n’ or < 0 if you error off the 0 case. The 0 case could be OK, if you allow zero to be return on a
>> empty ring for the fixed ring case.
>>>>> 
>>>>> Does this make sense to anyone?
>>>> It won't make sense unless you're aware of the history behind these functions. The original functions that were implemented for
>> the ring were only the bulk functions (i.e. FIXED). They would return exactly the number of items requested for dequeue (0 if success,
>> negative if error), and not return any if the required number were not available.
>>>> 
>>>> The burst (i.e. VARIABLE) functions came in much later (think it was r1.3 where we introduced them), and by that time, there were
>> already quite a number of deployments of DPDK in the field using the legacy ring functions. Therefore we made the decision to keep
>> the legacy behavior intact & not impacting deployed code - and merging the burst functions into the code. Given that there was no
>> "versioning" of the API/ABI in those releases :).
>>> 
>>> I see why the code is this way. If the developers used ‘if ( ret == 0 ) { /* do something */ }’ then it would break if it returned a
>> positive value on success. I would expect the normal behavior to be ‘if ( ret < 0 ) { /* error case */ }’ and fall thru for the success case. I
>> would love to change the code to just return <0 on error or >= 0 on success. I wonder how many customers code would break
>> changing the code to do just just the two steps. I think it will remove some code in a couple places that were testing for FIXED or
>> VARIABLE?
>>>> 
>>>> Hope that helps.
>>>> -Venky
>>>> 
>>>>> 
>>>>> Thanks
>>>>> ++Keith
>>>>> 
>>>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>> 
>>> Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533
>>> 
>> 
>> Since we are looking at making considerable ABI changes in this release and
>> (hopefully) also looking to version our ABI going forward, I would be in
>> favour of making any changes to these APIs in this current release if
>> possible. While the current behaviour makes sense for historical reason, I
>> think an overall change to the behaviour as Keith describes would be more
>> sensible long-term.
> 
> It is doable, I suppose, but might become quite messy:
> Don't know how many people are using  rte_ring_dequeue_bulk() all over the place.
> I suspect quite a lot.
> From other side - what the real gain we'll have from it?
> I don't see much so far.
> Konstantin
> 
I see two possible gains one is a consistent return method for Fixed/Variable and some code reduction in a few places. Let me see if I can create a patch we can review and see if it seems reasonable.

>> 
>> (Also to note my previous suggestion about upping the major version to 2.0
>> if we continue to increase the number of ABI/API changes in this release.
>> Anyone else any thoughts on that?)

Keith Wiles, Principal Technologist with CTO office, Wind River mobile 972-213-5533