Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR

[Santosh Shukla <sshukla@mvista.com>]

On Mon, Feb 22, 2016 at 7:33 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 2/19/2016 2:42 PM, Yuanhan Liu wrote:
>> On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
>>> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
>>> <yuanhan.liu@linux.intel.com> wrote:
>>>> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
>>>>> Hi Yuanhan,
>>>>>
>>>>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
>>>>> <yuanhan.liu@linux.intel.com> wrote:
>>>>>> On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
>>>>>>> Hi Yuanhan,
>>>>>>>
>>>>>>> I guess you are back from vacation.
>>>>>>>
>>>>>>> Can you pl. review this patch, Except this patch, rest of patches
>>>>>>> received ack-by:
>>>>>> I had a quick glimpse of the comments from Thomas: he made a good point.
>>>>>> I will have a deeper thought tomorrow, to see what I can do to fix it.
>>>>>>
>>>>> I agree to what Thomas pointed out about runtime mode switch (vectored
>>>>> vs non-vectored). I have a proposal in my mind and Like to know you
>>>>> opinion:
>>>>>
>>>>> - need for apis like is_arch_support_vec().
>>>>>
>>>>> if (is_arch_support_vec())
>>>>>          simpple_xxxx = 1 /* Switch code path to vector mode */
>>>>> else
>>>>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
>>>>>
>>>>> That api should reside to arch file. i.e.. arch like i686/arm{for
>>>>> implementation not exist so say no supported} will return 0 and for
>>>>> x86_64 = 1
>>>> I was thinking that Thomas meant to something like below (like what
>>>> we did at rte_memcpy.h):
>>>>
>>>>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
>>>>
>>>>         /* with vec here */
>>>>
>>>>     #else
>>>>
>>>>         /* without vec here */
>>>>
>>>>     #endif
>>>>
>>>> I mean, you have to bypass the build first; otherwise, you can't
>>>> go that further to runtime, right?
>>>>
>>> I meant: move virtio_recv_pkt_vec() implementation in
>>> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
>>> for CPUFLAG supported or not and then use _recv_pkt() call back
>>> function from arch files. This approach will avoid #ifdef ARCH
>>> clutter.
>> Moving virtio stuff to eal looks wrong to me.
>>
>> Huawei, please comment on this.
>>
>>       --yliu
>>
>
> This issue doesn't apply to virtio driver only but to all other PMDs,
> unless they are assumed to run on only one arch. As we are close to
> release, for the time being, i prefer to use RTE_MACHINE_CPUFLAG_. Later
> we look for other elegant solutions, like moving different arch specific
> optimizations into the arch directory under driver/virtio/ directory?
> Other thoughts?
>

Creating arch specifics files in driver/virtio/: approach look okay to
me. It look alike to my proposal except eal. I choose eal so that one
api and its implementation stays in arch files, no ifdef clutter. I
guess - Same doable in virtio directory too, create arch files and
keep arch specific implementation their.

so, +1 to approach.
>
>