On 2021/9/23 1:29, Jerin Jacob wrote:
> On Wed, Sep 22, 2021 at 10:06 PM Bruce Richardson
> <bruce.richard...@intel.com> wrote:
>>
>> On Wed, Sep 22, 2021 at 08:56:50AM +0100, Bruce Richardson wrote:
>>> On Wed, Sep 22, 2021 at 09:51:42AM +0800, fengchengwen wrote:
>>>> On 2021/9/22 2:11, Jerin Jacob wrote:
>>>>> On Tue, Sep 21, 2021 at 10:42 PM Pai G, Sunil <sunil.pa...@intel.com>
>>>>> wrote:
>>>>>>
>>>>>> Hi Jerin,
>>>>>>
>>>>>> <snipped>
>>>>>>
>>>>>>>>> To understand it better, Could you share more details on feedback
>>>>>>>>> mechanism on your application enqueue
>>>>>>>>>
>>>>>>>>> app_enqueue_v1(.., nb_seg)
>>>>>>>>> {
>>>>>>>>> /* Not enough space, Let application handle it by
>>>>>>>>> dropping or resubmitting */
>>>>>>>>> if (rte_dmadev_burst_capacity() < nb_seg)
>>>>>>>>> return -ENOSPC;
>>>>>>>>>
>>>>>>>>> do rte_dma_op() in loop without checking error;
>>>>>>>>> return 0; // Success
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> vs
>>>>>>>>> app_enqueue_v2(.., nb_seg)
>>>>>>>>> {
>>>>>>>>> int rc;
>>>>>>>>>
>>>>>>>>> rc |= rte_dma_op() in loop without checking error;
>>>>>>>>> return rc; // return the actual status to application
>>>>>>>>> if Not enough space, Let application handle it by dropping or
>>>>>>>>> resubmitting */ }
>>>>>>>>>
>>>>>>>>> Is app_enqueue_v1() and app_enqueue_v2() logically the same from
>>>>>>>>> application PoV. Right?
>>>>>>>>>
>>>>>>>>> If not, could you explain, the version you are planning to do for
>>>>>>>>> app_enqueue()
>>>>>>>>
>>>>>>>> The exact version can be found here :
>>>>>>>>
>>>>>>> http://patchwork.ozlabs.org/project/openvswitch/patch/20210907120021.4
>>>>>>>> 093604-2-sunil.pa...@intel.com/ Unfortunately, both versions are not
>>>>>>>> same in our case because of the SW fallback we have for ring full
>>>>>>>> scenario's.
>>>>>>>> For a packet with 8 nb_segs, if the ring has only space for 4 , we
>>>>>>>> would avoid this packet with app_enqueue_v1 while going ahead with an
>>>>>>> enqueue with app_enqueue_v2, resulting in a mix of DMA and CPU copies
>>>>>>> for a packet which we would want to avoid.
>>>>>>>
>>>>>>> Thanks for RFC link. Usage is clear now, Since you are checking the
>>>>>>> space in
>>>>>>> the completion handler, I am not sure, what is hard to get the remaining
>>>>>>> space, Will following logic work to find the empty space. If not,
>>>>>>> please discuss
>>>>>>> the issue with this approach. I am trying to avoid yet another fastpath
>>>>>>> API
>>>>>>> and complication in driver as there is element checking space in the
>>>>>>> submit
>>>>>>> queue and completion queue at least in our hardware.
>>>>>>>
>>>>>>> max_count = nb_desc; (power of 2)
>>>>>>> mask = max_count - 1;
>>>>>>>
>>>>>>> for (i = 0; I < n; i++) {
>>>>>>> submit_idx = rte_dma_copy();
>>>>>>> }
>>>>>>> rc = rte_dma_completed(…, &completed_idx..);
>>>>>>> space_in_queue = mask - ((submit_idx – completed_idx) & mask);
>>>>>>>
>>>>>>
>>>>>> Unfortunately, space left in the device (as calculated by the app) still
>>>>>> can mean there is no space in the device :|
>>>>>> i.e its pmd dependent.
>>>>>
>>>>> I did not pay much attention to Jiayu's reply as I did not know what is
>>>>> DSA.
>>>>> Now I searched the DSA in ml, I could see the PMD patches.
>>>>>
>>>>> If it is PMD limitation then I am fine with the proposed API.
>>>>>
>>>>> @Richardson, Bruce @Laatz, Kevin @feng Since it is used fastpath, Can
>>>>> we move to fastpath handler and
>>>>> remove additional check in fastpath from common code like other APIs.
>>>>
>>>> +1 for move to fastpath.
>>>>
>>>
>>> Will move in next revision.
>>
>> Follow-up question on this. If it's a fastpath function then we would not
>> normally check for support from drivers. Therefore do we want to:
>> 1. make it a mandatory function
>> 2. add a feature capability flag
>>
>> Given that it's likely fairly easy for all drivers to implement, and it
>> makes it easier for apps to avoid having to check a feature flag for, I'd
>> tend towards option #1, but just would like consensus before I push any
>> more patches.
>
> I think, if vhost using in that way then it needs to make it a
> mandatory function.
+1
>
>>
>> /Bruce
> .
>
