Re: [dpdk-dev] [RFC v2] doc compression API for DPDK

["Verma, Shally" <Shally.Verma@cavium.com>]

>-----Original Message-----
>From: Ahmed Mansour [mailto:ahmed.mansour@nxp.com]
>Sent: 22 February 2018 01:06
>To: Trahe, Fiona <fiona.trahe@intel.com>; Verma, Shally <Shally.Verma@cavium.com>; dev@dpdk.org
>Cc: Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Gupta, Ashish <Ashish.Gupta@cavium.com>; Sahu, Sunila
><Sunila.Sahu@cavium.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Challa, Mahipal
><Mahipal.Challa@cavium.com>; Jain, Deepak K <deepak.k.jain@intel.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Roy
>Pledge <roy.pledge@nxp.com>; Youri Querry <youri.querry_1@nxp.com>
>Subject: Re: [RFC v2] doc compression API for DPDK
>
>On 2/21/2018 9:35 AM, Trahe, Fiona wrote:
>> Hi Ahmed, Shally,
>>
>>
>>> -----Original Message-----
>>> From: Ahmed Mansour [mailto:ahmed.mansour@nxp.com]
>>> Sent: Tuesday, February 20, 2018 7:56 PM
>>> To: Verma, Shally <Shally.Verma@cavium.com>; Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org
>>> Cc: Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Gupta, Ashish
>>> <Ashish.Gupta@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; De Lara Guarch, Pablo
>>> <pablo.de.lara.guarch@intel.com>; Challa, Mahipal <Mahipal.Challa@cavium.com>; Jain, Deepak K
>>> <deepak.k.jain@intel.com>; Hemant Agrawal <hemant.agrawal@nxp.com>; Roy Pledge
>>> <roy.pledge@nxp.com>; Youri Querry <youri.querry_1@nxp.com>
>>> Subject: Re: [RFC v2] doc compression API for DPDK
>>>
>>> /// snip ///
>>>>>>>>>>>>>>>>>> D.2.1 Stateful operation state maintenance
>>>>>>>>>>>>>>>>>> ---------------------------------------------------------------
>>>>>>>>>>>>>>>>>> It is always an ideal expectation from application that it should parse
>>>>>>>>>>>>>>>>> through all related chunk of source data making its mbuf-chain and
>>>>>>>>>>>>>>> enqueue
>>>>>>>>>>>>>>>>> it for stateless processing.
>>>>>>>>>>>>>>>>>> However, if it need to break it into several enqueue_burst() calls, then
>>>>>>>>>>>>>>> an
>>>>>>>>>>>>>>>>> expected call flow would be something like:
>>>>>>>>>>>>>>>>>> enqueue_burst( |op.no_flush |)
>>>>>>>>>>>>>>>>> [Ahmed] The work is now in flight to the PMD.The user will call dequeue
>>>>>>>>>>>>>>>>> burst in a loop until all ops are received. Is this correct?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> deque_burst(op) // should dequeue before we enqueue next
>>>>>>>>>>>>>>>> [Shally] Yes. Ideally every submitted op need to be dequeued. However
>>>>>>>>>>>>>>> this illustration is specifically in
>>>>>>>>>>>>>>>> context of stateful op processing to reflect if a stream is broken into
>>>>>>>>>>>>>>> chunks, then each chunk should be
>>>>>>>>>>>>>>>> submitted as one op at-a-time with type = STATEFUL and need to be
>>>>>>>>>>>>>>> dequeued first before next chunk is
>>>>>>>>>>>>>>>> enqueued.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> enqueue_burst( |op.no_flush |)
>>>>>>>>>>>>>>>>>> deque_burst(op) // should dequeue before we enqueue next
>>>>>>>>>>>>>>>>>> enqueue_burst( |op.full_flush |)
>>>>>>>>>>>>>>>>> [Ahmed] Why now allow multiple work items in flight? I understand that
>>>>>>>>>>>>>>>>> occasionaly there will be OUT_OF_SPACE exception. Can we just
>>>>>>>>>>>>>>> distinguish
>>>>>>>>>>>>>>>>> the response in exception cases?
>>>>>>>>>>>>>>>> [Shally] Multiples ops are allowed in flight, however condition is each op in
>>>>>>>>>>>>>>> such case is independent of
>>>>>>>>>>>>>>>> each other i.e. belong to different streams altogether.
>>>>>>>>>>>>>>>> Earlier (as part of RFC v1 doc) we did consider the proposal to process all
>>>>>>>>>>>>>>> related chunks of data in single
>>>>>>>>>>>>>>>> burst by passing them as ops array but later found that as not-so-useful for
>>>>>>>>>>>>>>> PMD handling for various
>>>>>>>>>>>>>>>> reasons. You may please refer to RFC v1 doc review comments for same.
>>>>>>>>>>>>>>> [Fiona] Agree with Shally. In summary, as only one op can be processed at a
>>>>>>>>>>>>>>> time, since each needs the
>>>>>>>>>>>>>>> state of the previous, to allow more than 1 op to be in-flight at a time would
>>>>>>>>>>>>>>> force PMDs to implement internal queueing and exception handling for
>>>>>>>>>>>>>>> OUT_OF_SPACE conditions you mention.
>>>>>>>>>>>>> [Ahmed] But we are putting the ops on qps which would make them
>>>>>>>>>>>>> sequential. Handling OUT_OF_SPACE conditions would be a little bit more
>>>>>>>>>>>>> complex but doable.
>>>>>>>>>>>> [Fiona] In my opinion this is not doable, could be very inefficient.
>>>>>>>>>>>> There may be many streams.
>>>>>>>>>>>> The PMD would have to have an internal queue per stream so
>>>>>>>>>>>> it could adjust the next src offset and length in the OUT_OF_SPACE case.
>>>>>>>>>>>> And this may ripple back though all subsequent ops in the stream as each
>>>>>>>>>>>> source len is increased and its dst buffer is not big enough.
>>>>>>>>>>> [Ahmed] Regarding multi op OUT_OF_SPACE handling.
>>>>>>>>>>> The caller would still need to adjust
>>>>>>>>>>> the src length/output buffer as you say. The PMD cannot handle
>>>>>>>>>>> OUT_OF_SPACE internally.
>>>>>>>>>>> After OUT_OF_SPACE occurs, the PMD should reject all ops in this stream
>>>>>>>>>>> until it gets explicit
>>>>>>>>>>> confirmation from the caller to continue working on this stream. Any ops
>>>>>>>>>>> received by
>>>>>>>>>>> the PMD should be returned to the caller with status STREAM_PAUSED since
>>>>>>>>>>> the caller did not
>>>>>>>>>>> explicitly acknowledge that it has solved the OUT_OF_SPACE issue.
>>>>>>>>>>> These semantics can be enabled by adding a new function to the API
>>>>>>>>>>> perhaps stream_resume().
>>>>>>>>>>> This allows the caller to indicate that it acknowledges that it has seen
>>>>>>>>>>> the issue and this op
>>>>>>>>>>> should be used to resolve the issue. Implementations that do not support
>>>>>>>>>>> this mode of use
>>>>>>>>>>> can push back immediately after one op is in flight. Implementations
>>>>>>>>>>> that support this use
>>>>>>>>>>> mode can allow many ops from the same session
>>>>>>>>>>>
>>>>>>>>>> [Shally] Is it still in context of having single burst where all op belongs to one stream? If yes, I
>>> would
>>>>>>> still
>>>>>>>>>> say it would add an overhead to PMDs especially if it is expected to work closer to HW (which I
>>> think
>>>>>>> is
>>>>>>>>>> the case with DPDK PMD).
>>>>>>>>>> Though your approach is doable but why this all cannot be in a layer above PMD? i.e. a layer
>>> above
>>>>>>> PMD
>>>>>>>>>> can either pass one-op at a time with burst size = 1 OR can make chained mbuf of input and
>>> output
>>>>>>> and
>>>>>>>>>> pass than as one op.
>>>>>>>>>> Is it just to ease applications of chained mbuf burden or do you see any performance /use-case
>>>>>>>>>> impacting aspect also?
>>>>>>>>>>
>>>>>>>>>> if it is in context where each op belong to different stream in a burst, then why do we need
>>>>>>>>>> stream_pause and resume? It is a expectations from app to pass more output buffer with
>>> consumed
>>>>>>> + 1
>>>>>>>>>> from next call onwards as it has already
>>>>>>>>>> seen OUT_OF_SPACE.
>>>>>>>> [Ahmed] Yes, this would add extra overhead to the PMD. Our PMD
>>>>>>>> implementation rejects all ops that belong to a stream that has entered
>>>>>>>> "RECOVERABLE" state for one reason or another. The caller must
>>>>>>>> acknowledge explicitly that it has received news of the problem before
>>>>>>>> the PMD allows this stream to exit "RECOVERABLE" state. I agree with you
>>>>>>>> that implementing this functionality in the software layer above the PMD
>>>>>>>> is a bad idea since the latency reductions are lost.
>>>>>>> [Shally] Just reiterating, I rather meant other way around i.e. I see it easier to put all such complexity
>>> in a
>>>>>>> layer above PMD.
>>>>>>>
>>>>>>>> This setup is useful in latency sensitive applications where the latency
>>>>>>>> of buffering multiple ops into one op is significant. We found latency
>>>>>>>> makes a significant difference in search applications where the PMD
>>>>>>>> competes with software decompression.
>>>>>> [Fiona] I see, so when all goes well, you get best-case latency, but when
>>>>>> out-of-space occurs latency will probably be worse.
>>>>> [Ahmed] This is exactly right. This use mode assumes out-of-space is a
>>>>> rare occurrence. Recovering from it should take similar time to
>>>>> synchronous implementations. The caller gets OUT_OF_SPACE_RECOVERABLE in
>>>>> both sync and async use. The caller can fix up the op and send it back
>>>>> to the PMD to continue work just as would be done in sync. Nonetheless,
>>>>> the added complexity is not justifiable if out-of-space is very common
>>>>> since the recoverable state will be the limiting factor that forces
>>>>> synchronicity.
>>>>>>>>> [Fiona] I still have concerns with this and would not want to support in our PMD.
>>>>>>>>> TO make sure I understand, you want to send a burst of ops, with several from same stream.
>>>>>>>>> If one causes OUT_OF_SPACE_RECOVERABLE, then the PMD should not process any
>>>>>>>>> subsequent ops in that stream.
>>>>>>>>> Should it return them in a dequeue_burst() with status still NOT_PROCESSED?
>>>>>>>>> Or somehow drop them? How?
>>>>>>>>> While still processing ops form other streams.
>>>>>>>> [Ahmed] This is exactly correct. It should return them with
>>>>>>>> NOT_PROCESSED. Yes, the PMD should continue processing other streams.
>>>>>>>>> As we want to offload each op to hardware with as little CPU processing as possible we
>>>>>>>>> would not want to open up each op to see which stream it's attached to and
>>>>>>>>> make decisions to do per-stream storage, or drop it, or bypass hw and dequeue without
>>> processing.
>>>>>>>> [Ahmed] I think I might have missed your point here, but I will try to
>>>>>>>> answer. There is no need to "cushion" ops in DPDK. DPDK should send ops
>>>>>>>> to the PMD and the PMD should reject until stream_continue() is called.
>>>>>>>> The next op to be sent by the user will have a special marker in it to
>>>>>>>> inform the PMD to continue working on this stream. Alternatively the
>>>>>>>> DPDK layer can be made "smarter" to fail during the enqueue by checking
>>>>>>>> the stream and its state, but like you say this adds additional CPU
>>>>>>>> overhead during the enqueue.
>>>>>>>> I am curious. In a simple synchronous use case. How do we prevent users
>>>>>>> >from putting multiple ops in flight that belong to a single stream? Do
>>>>>>>> we just currently say it is undefined behavior? Otherwise we would have
>>>>>>>> to check the stream and incur the CPU overhead.
>>>>>> [Fiona] We don't do anything to prevent it. It's undefined. IMO on data path in
>>>>>> DPDK model we expect good behaviour and don't have to error check for things like this.
>>>>> [Ahmed] This makes sense. We also assume good behavior.
>>>>>> In our PMD if we got a burst of 20 ops, we allocate 20 spaces on the hw q, then
>>>>>> build and send those messages. If we found an op from a stream which already
>>>>>> had one inflight, we'd have to hold that back, store in a sw stream-specific holding queue,
>>>>>> only send 19 to hw. We cannot send multiple ops from same stream to
>>>>>> the hw as it fans them out and does them in parallel.
>>>>>> Once the enqueue_burst() returns, there is no processing
>>>>>> context which would spot that the first has completed
>>>>>> and send the next op to the hw. On a dequeue_burst() we would spot this,
>>>>>> in that context could process the next op in the stream.
>>>>>> On out of space, instead of processing the next op we would have to transfer
>>>>>> all unprocessed ops from the stream to the dequeue result.
>>>>>> Some parts of this are doable, but seems likely to add a lot more latency,
>>>>>> we'd need to add extra threads and timers to move ops from the sw
>>>>>> queue to the hw q to get any benefit, and these constructs would add
>>>>>> context switching and CPU cycles. So we prefer to push this responsibility
>>>>>> to above the API and it can achieve similar.
>>>>> [Ahmed] I see what you mean. Our workflow is almost exactly the same
>>>>> with our hardware, but the fanning out is done by the hardware based on
>>>>> the stream and ops that belong to the same stream are never allowed to
>>>>> go out of order. Otherwise the data would be corrupted. Likewise the
>>>>> hardware is responsible for checking the state of the stream and
>>>>> returning frames as NOT_PROCESSED to the software
>>>>>>>>> Maybe we could add a capability if this behaviour is important for you?
>>>>>>>>> e.g. ALLOW_ENQUEUE_MULTIPLE_STATEFUL_OPS ?
>>>>>>>>> Our PMD would set this to 0. And expect no more than one op from a stateful stream
>>>>>>>>> to be in flight at any time.
>>>>>>>> [Ahmed] That makes sense. This way the different DPDK implementations do
>>>>>>>> not have to add extra checking for unsupported cases.
>>>>>>> [Shally] @ahmed, If I summarise your use-case, this is how to want to PMD to support?
>>>>>>> - a burst *carry only one stream* and all ops then assumed to be belong to that stream? (please
>>> note,
>>>>>>> here burst is not carrying more than one stream)
>>>>> [Ahmed] No. In this use case the caller sets up an op and enqueues a
>>>>> single op. Then before the response comes back from the PMD the caller
>>>>> enqueues a second op on the same stream.
>>>>>>> -PMD will submit one op at a time to HW?
>>>>> [Ahmed] I misunderstood what PMD means. I used it throughout to mean the
>>>>> HW. I used DPDK to mean the software implementation that talks to the
>>>>> hardware.
>>>>> The software will submit all ops immediately. The hardware has to figure
>>>>> out what to do with the ops depending on what stream they belong to.
>>>>>>> -if processed successfully, push it back to completion queue with status = SUCCESS. If failed or run to
>>>>>>> into OUT_OF_SPACE, then push it to completion queue with status = FAILURE/
>>>>>>> OUT_OF_SPACE_RECOVERABLE and rest with status = NOT_PROCESSED and return with enqueue
>>> count
>>>>>>> = total # of ops submitted originally with burst?
>>>>> [Ahmed] This is exactly what I had in mind. all ops will be submitted to
>>>>> the HW. The HW will put all of them on the completion queue with the
>>>>> correct status exactly as you say.
>>>>>>> -app assumes all have been enqueued, so it go and dequeue all ops
>>>>>>> -on seeing an op with OUT_OF_SPACE_RECOVERABLE, app resubmit a burst of ops with call to
>>>>>>> stream_continue/resume API starting from op which encountered OUT_OF_SPACE and others as
>>>>>>> NOT_PROCESSED with updated input and output buffer?
>>>>> [Ahmed] Correct this is what we do today in our proprietary API.
>>>>>>> -repeat until *all* are dequeued with status = SUCCESS or *any* with status = FAILURE? If anytime
>>>>>>> failure is seen, then app start whole processing all over again or just drop this burst?!
>>>>> [Ahmed] The app has the choice on how to proceed. If the issue is
>>>>> recoverable then the application can continue this stream from where it
>>>>> stopped. if the failure is unrecoverable then the application should
>>>>> first fix the problem and start from the beginning of the stream.
>>>>>>> If all of above is true, then I think we should add another API such as
>>> rte_comp_enque_single_stream()
>>>>>>> which will be functional under Feature Flag = ALLOW_ENQUEUE_MULTIPLE_STATEFUL_OPS or better
>>>>>>> name is SUPPORT_ENQUEUE_SINGLE_STREAM?!
>>>>> [Ahmed] The main advantage in async use is lost if we force all related
>>>>> ops to be in the same burst. if we do that, then we might as well merge
>>>>> all the ops into one op. That would reduce the overhead.
>>>>> The use mode I am proposing is only useful in cases where the data
>>>>> becomes available after the first enqueue occurred. I want to allow the
>>>>> caller to enqueue the second set of data as soon as it is available
>>>>> regardless of whether or not the HW has already started working on the
>>>>> first op inflight.
>>>> [Shally] @ahmed,  Ok.. seems I missed a point here. So, confirm me following:
>>>>
>>>> As per current description in doc, expected stateful usage is:
>>>> enqueue (op1) --> dequeue(op1) --> enqueue(op2)
>>>>
>>>> but you're suggesting to allow an option to change it to
>>>>
>>>> enqueue(op1) -->enqueue(op2)
>>>>
>>>> i.e.  multiple ops from same stream can be put in-flight via subsequent enqueue_burst() calls without
>>> waiting to dequeue previous ones as PMD support it . So, no change to current definition of a burst. It will
>>> still carry multiple streams where each op belonging to different stream ?!
>>> [Ahmed] Correct. I guess a user could put two ops on the same burst that
>>> belong to the same stream. In that case it would be more efficient to
>>> merge the ops using scatter gather. Nonetheless, I would not add checks
>>> in my implementation to limit that use. The hardware does not perceive a
>>> difference between ops that came on one burst and ops that came on two
>>> different bursts. to the hardware they are all ops. What matters is
>>> which stream each op belongs to.
>>>> if yes, then seems your HW can be setup for multiple streams so it is efficient for your case to support it
>>> in DPDK PMD layer but our hw doesn't by-default and need SW to back it. Given that, I also suggest to
>>> enable it under some feature flag.
>>>> However it looks like an add-on and if it doesn't change current definition of a burst and minimum
>>> expectation set on stateful processing described in this document, then IMO, you can propose this feature
>>> as an incremental patch on baseline version, in absence of which,
>>>> application will exercise stateful processing as described here (enq->deq->enq). Thoughts?
>>> [Ahmed] Makes sense. I was worried that there might be fundamental
>>> limitations to this mode of use in the API design. That is why I wanted
>>> to share this use mode with you guys and see if it can be accommodated
>>> using an incremental patch in the future.
>>>>>> [Fiona] Am curious about Ahmed's response to this. I didn't get that a burst should carry only one
>>> stream
>>>>>> Or get how this makes a difference? As there can be many enqueue_burst() calls done before an
>>> dequeue_burst()
>>>>>> Maybe you're thinking the enqueue_burst() would be a blocking call that would not return until all the
>>> ops
>>>>>> had been processed? This would turn it into a synchronous call which isn't the intent.
>>>>> [Ahmed] Agreed, a blocking or even a buffering software layer that baby
>>>>> sits the hardware does not fundamentally change the parameters of the
>>>>> system as a whole. It just moves workflow management complexity down
>>>>> into the DPDK software layer. Rather there are real latency and
>>>>> throughput advantages (because of caching) that I want to expose.
>>>>>
>> [Fiona] ok, so I think we've agreed that this can be an option, as long as not required of
>> PMDs and enabled under an explicit capability - named something like
>> ALLOW_ENQUEUE_MULTIPLE_STATEFUL_OPS
>> @Ahmed, we'll leave it up to you to define details.
>> What's necessary is API text to describe the expected behaviour on any error conditions,
>> the pause/resume API, whether an API is expected to clean up if resume doesn't happen
>> and if there's any time limit on this, etc
>> But I wouldn't expect any changes to existing burst APIs, and all PMDs and applications
>> must be able to handle the default behaviour, i.e. with this capability disabled.
>> Specifically even if a PMD has this capability, if an application ignores it and only sends
>> one op at a time, if a PMD returns OUT_OF_SPACE_RECOVERABLE the stream should
>> not be in a paused state and the PMD should not wait for a resume() to handle the
>> next op sent for that stream.
>> Does that make sense?
>[Ahmed] That make sense. When this mode is enabled then additional
>functions must be called to resume the work, even if only one op was in
>flight. When this mode is not enabled then the PMD assumes that the
>caller will never enqueue a stateful op before receiving a response to
>the one that precedes it in a stream

[Shally] @ahmed , just to confirm on this

>When this mode is not enabled then the PMD assumes that the caller will never enqueue a stateful op ...

I think what we want to ensure reverse of it i.e. "if mode is *enabled*, then also PMD should assume that caller can use enqueue->dequeue->enqueue sequence for stateful processing and if on deque, 
he discover OUT_OF_SPACE_RECOVERABLE and call enqueue() again to handle it , that should be also be supported by PMD" . 
In a sense, an application written for one PMD which doesn't have this capability should also work for PMD which has this capability.

>>
>>>>> /// snip ///
>>>>
>>