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.
>
> /Bruce
