[dpdk-dev,v1] virtio: Use cpuflag for vector api
Commit Message
Check cpuflag macro before using vectored api.
-virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
- Also wrap other vectored freind api ie..
1) virtqueue_enqueue_recv_refill_simple
2) virtio_rxq_vec_setup
todo:
1) Move virtio_recv_pkts_vec() implementation to
drivers/virtio/virtio_vec_<arch>.h file.
2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
files to provide vectored/non-vectored rx/tx apis.
Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
- v1: This is a rework of patch [1].
Note: This patch will let non-x86 arch to use virtio pmd.
[1] http://dpdk.org/dev/patchwork/patch/10429/
drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
drivers/net/virtio/virtio_rxtx.h | 2 ++
drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
3 files changed, 27 insertions(+), 2 deletions(-)
Comments
On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
> Check cpuflag macro before using vectored api.
> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> - Also wrap other vectored freind api ie..
> 1) virtqueue_enqueue_recv_refill_simple
> 2) virtio_rxq_vec_setup
>
...
> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
> index 3a1de9d..be51d7c 100644
> --- a/drivers/net/virtio/virtio_rxtx_simple.c
> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
Hmm, why not wrapping the whole file, instead of just few functions?
Or maybe better, do a compile time check at the Makefile, something
like:
if has_CPUFLAG_xxx
SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
endif
--yliu
On 2/29/2016 12:26 PM, Yuanhan Liu wrote:
> On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
>> Check cpuflag macro before using vectored api.
>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>> - Also wrap other vectored freind api ie..
>> 1) virtqueue_enqueue_recv_refill_simple
>> 2) virtio_rxq_vec_setup
>>
> ...
>> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
>> index 3a1de9d..be51d7c 100644
>> --- a/drivers/net/virtio/virtio_rxtx_simple.c
>> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
> Hmm, why not wrapping the whole file, instead of just few functions?
>
> Or maybe better, do a compile time check at the Makefile, something
> like:
>
> if has_CPUFLAG_xxx
> SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
> endif
>
>
> --yliu
>
For next release, we could consider providing arch level framework for
different arch optimizations. It is more complicated for rte_memcpy. For
the time being, except the small issue, ok with the temporary solution
using CPUFLAG.
On Mon, Feb 29, 2016 at 9:57 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
>> Check cpuflag macro before using vectored api.
>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>> - Also wrap other vectored freind api ie..
>> 1) virtqueue_enqueue_recv_refill_simple
>> 2) virtio_rxq_vec_setup
>>
> ...
>> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
>> index 3a1de9d..be51d7c 100644
>> --- a/drivers/net/virtio/virtio_rxtx_simple.c
>> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
>
> Hmm, why not wrapping the whole file, instead of just few functions?
>
Better to refactor code and make arch specific. Current implementation
is temporary.
> Or maybe better, do a compile time check at the Makefile, something
> like:
>
> if has_CPUFLAG_xxx
> SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
> endif
>
Tried this approach but end up with link error, If I try to fix below
link error then I will be ending up writing similar code,
linker error snap:
/work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
In function `virtio_dev_rxtx_start':
virtio_rxtx.c:(.text+0x168c): undefined reference to
`virtqueue_enqueue_recv_refill_simple'
/work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
In function `virtio_dev_rx_queue_setup':
virtio_rxtx.c:(.text+0x2364): undefined reference to `virtio_rxq_vec_setup'
/work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
In function `virtio_dev_tx_queue_setup':
virtio_rxtx.c:(.text+0x2460): undefined reference to `virtio_xmit_pkts_simple'
virtio_rxtx.c:(.text+0x2464): undefined reference to `virtio_recv_pkts_vec'
virtio_rxtx.c:(.text+0x2468): undefined reference to `virtio_xmit_pkts_simple'
virtio_rxtx.c:(.text+0x246c): undefined reference to `virtio_recv_pkts_vec'
collect2: error: ld returned 1 exit status
make[5]: *** [test] Error 1
make[4]: *** [test] Error 2
make[3]: *** [app] Error 2
>
> --yliu
On Mon, Feb 29, 2016 at 06:01:38PM +0530, Santosh Shukla wrote:
> On Mon, Feb 29, 2016 at 9:57 AM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
> > On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
> >> Check cpuflag macro before using vectored api.
> >> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> >> - Also wrap other vectored freind api ie..
> >> 1) virtqueue_enqueue_recv_refill_simple
> >> 2) virtio_rxq_vec_setup
> >>
> > ...
> >> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
> >> index 3a1de9d..be51d7c 100644
> >> --- a/drivers/net/virtio/virtio_rxtx_simple.c
> >> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
> >
> > Hmm, why not wrapping the whole file, instead of just few functions?
> >
>
> Better to refactor code and make arch specific. Current implementation
> is temporary.
I'm okay to refactor, if it's a clean one. But moving virtio __driver__
stuff to EAL, sorry, it still doesn't make too much sense to me.
For this case, let's make it simple so far, and consider it when we
have another vec implementation, or better refactor comes out.
> > Or maybe better, do a compile time check at the Makefile, something
> > like:
> >
> > if has_CPUFLAG_xxx
> > SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
> > endif
> >
> Tried this approach but end up with link error, If I try to fix below
> link error then I will be ending up writing similar code,
Sure you need the first part of your patch. Yes, it's similar code,
but it has fewer "#ifdef " conditions, and more importantly, it
leaves virtio_rxtx_simple.c alone.
--yliu
> linker error snap:
>
> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
> In function `virtio_dev_rxtx_start':
> virtio_rxtx.c:(.text+0x168c): undefined reference to
> `virtqueue_enqueue_recv_refill_simple'
> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
> In function `virtio_dev_rx_queue_setup':
> virtio_rxtx.c:(.text+0x2364): undefined reference to `virtio_rxq_vec_setup'
> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
> In function `virtio_dev_tx_queue_setup':
> virtio_rxtx.c:(.text+0x2460): undefined reference to `virtio_xmit_pkts_simple'
> virtio_rxtx.c:(.text+0x2464): undefined reference to `virtio_recv_pkts_vec'
> virtio_rxtx.c:(.text+0x2468): undefined reference to `virtio_xmit_pkts_simple'
> virtio_rxtx.c:(.text+0x246c): undefined reference to `virtio_recv_pkts_vec'
On Tue, Mar 1, 2016 at 11:25 AM, Yuanhan Liu
<yuanhan.liu@linux.intel.com> wrote:
> On Mon, Feb 29, 2016 at 06:01:38PM +0530, Santosh Shukla wrote:
>> On Mon, Feb 29, 2016 at 9:57 AM, Yuanhan Liu
>> <yuanhan.liu@linux.intel.com> wrote:
>> > On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
>> >> Check cpuflag macro before using vectored api.
>> >> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>> >> - Also wrap other vectored freind api ie..
>> >> 1) virtqueue_enqueue_recv_refill_simple
>> >> 2) virtio_rxq_vec_setup
>> >>
>> > ...
>> >> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
>> >> index 3a1de9d..be51d7c 100644
>> >> --- a/drivers/net/virtio/virtio_rxtx_simple.c
>> >> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
>> >
>> > Hmm, why not wrapping the whole file, instead of just few functions?
>> >
>>
>> Better to refactor code and make arch specific. Current implementation
>> is temporary.
>
> I'm okay to refactor, if it's a clean one. But moving virtio __driver__
> stuff to EAL, sorry, it still doesn't make too much sense to me.
>
You misunderstood my comment, my arch specific meaning
driver/net/virtio/virtio_vec_<arch>.h
> For this case, let's make it simple so far, and consider it when we
> have another vec implementation, or better refactor comes out.
>
>> > Or maybe better, do a compile time check at the Makefile, something
>> > like:
>> >
>> > if has_CPUFLAG_xxx
>> > SRCS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio_rxtx_simple.c
>> > endif
>> >
>> Tried this approach but end up with link error, If I try to fix below
>> link error then I will be ending up writing similar code,
>
> Sure you need the first part of your patch. Yes, it's similar code,
> but it has fewer "#ifdef " conditions, and more importantly, it
> leaves virtio_rxtx_simple.c alone.
>
Ok.,
> --yliu
>
>> linker error snap:
>>
>> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
>> In function `virtio_dev_rxtx_start':
>> virtio_rxtx.c:(.text+0x168c): undefined reference to
>> `virtqueue_enqueue_recv_refill_simple'
>> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
>> In function `virtio_dev_rx_queue_setup':
>> virtio_rxtx.c:(.text+0x2364): undefined reference to `virtio_rxq_vec_setup'
>> /work/santosh/thunder/nfs/dpdk/arm64-thunderx-linuxapp-gcc/lib/librte_pmd_virtio.a(virtio_rxtx.o):
>> In function `virtio_dev_tx_queue_setup':
>> virtio_rxtx.c:(.text+0x2460): undefined reference to `virtio_xmit_pkts_simple'
>> virtio_rxtx.c:(.text+0x2464): undefined reference to `virtio_recv_pkts_vec'
>> virtio_rxtx.c:(.text+0x2468): undefined reference to `virtio_xmit_pkts_simple'
>> virtio_rxtx.c:(.text+0x246c): undefined reference to `virtio_recv_pkts_vec'
On Tue, Mar 01, 2016 at 11:40:41AM +0530, Santosh Shukla wrote:
> On Tue, Mar 1, 2016 at 11:25 AM, Yuanhan Liu
> <yuanhan.liu@linux.intel.com> wrote:
> > On Mon, Feb 29, 2016 at 06:01:38PM +0530, Santosh Shukla wrote:
> >> On Mon, Feb 29, 2016 at 9:57 AM, Yuanhan Liu
> >> <yuanhan.liu@linux.intel.com> wrote:
> >> > On Fri, Feb 26, 2016 at 02:21:02PM +0530, Santosh Shukla wrote:
> >> >> Check cpuflag macro before using vectored api.
> >> >> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> >> >> - Also wrap other vectored freind api ie..
> >> >> 1) virtqueue_enqueue_recv_refill_simple
> >> >> 2) virtio_rxq_vec_setup
> >> >>
> >> > ...
> >> >> diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
> >> >> index 3a1de9d..be51d7c 100644
> >> >> --- a/drivers/net/virtio/virtio_rxtx_simple.c
> >> >> +++ b/drivers/net/virtio/virtio_rxtx_simple.c
> >> >
> >> > Hmm, why not wrapping the whole file, instead of just few functions?
> >> >
> >>
> >> Better to refactor code and make arch specific. Current implementation
> >> is temporary.
> >
> > I'm okay to refactor, if it's a clean one. But moving virtio __driver__
> > stuff to EAL, sorry, it still doesn't make too much sense to me.
> >
>
> You misunderstood my comment, my arch specific meaning
> driver/net/virtio/virtio_vec_<arch>.h
Oh, sorry. Then, yes, that's the way to go, if refactor is really
needed.
--yliu
On 2/26/2016 4:53 PM, Santosh Shukla wrote:
> Check cpuflag macro before using vectored api.
> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> - Also wrap other vectored freind api ie..
> 1) virtqueue_enqueue_recv_refill_simple
> 2) virtio_rxq_vec_setup
>
> todo:
> 1) Move virtio_recv_pkts_vec() implementation to
> drivers/virtio/virtio_vec_<arch>.h file.
> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
> files to provide vectored/non-vectored rx/tx apis.
>
> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> ---
> - v1: This is a rework of patch [1].
> Note: This patch will let non-x86 arch to use virtio pmd.
>
> [1] http://dpdk.org/dev/patchwork/patch/10429/
>
> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
> drivers/net/virtio/virtio_rxtx.h | 2 ++
> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 41a1366..ec0b8de 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -67,7 +67,9 @@
> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
> ETH_TXQ_FLAGS_NOOFFLOADS)
>
> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
> static int use_simple_rxtx;
> +#endif
>
>
I don't think so much #ifdef ... #endif in *.c file is a good choice.
Would you consider let it only in header file like:
in drivers/net/virtio/virtio_rxtx.h
[...]
#ifdef RTE_MACHINE_CPUFLAG_SSSE3
int virtio_rxq_vec_setup(struct virtqueue *rxq);
int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
struct rte_mbuf *m);
#else
int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
__rte_unused struct rte_mbuf *m) {
return -1;
}
#endif
and remove most #ifdef ... #endif in *.c file.
Thanks,
Michael
On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael <michael.qiu@intel.com> wrote:
> On 2/26/2016 4:53 PM, Santosh Shukla wrote:
>> Check cpuflag macro before using vectored api.
>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>> - Also wrap other vectored freind api ie..
>> 1) virtqueue_enqueue_recv_refill_simple
>> 2) virtio_rxq_vec_setup
>>
>> todo:
>> 1) Move virtio_recv_pkts_vec() implementation to
>> drivers/virtio/virtio_vec_<arch>.h file.
>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
>> files to provide vectored/non-vectored rx/tx apis.
>>
>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>> ---
>> - v1: This is a rework of patch [1].
>> Note: This patch will let non-x86 arch to use virtio pmd.
>>
>> [1] http://dpdk.org/dev/patchwork/patch/10429/
>>
>> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
>> drivers/net/virtio/virtio_rxtx.h | 2 ++
>> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
>> 3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index 41a1366..ec0b8de 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -67,7 +67,9 @@
>> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
>> ETH_TXQ_FLAGS_NOOFFLOADS)
>>
>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
>> static int use_simple_rxtx;
>> +#endif
>>
>>
>
> I don't think so much #ifdef ... #endif in *.c file is a good choice.
> Would you consider let it only in header file like:
>
> in drivers/net/virtio/virtio_rxtx.h
>
> [...]
>
> #ifdef RTE_MACHINE_CPUFLAG_SSSE3
> int virtio_rxq_vec_setup(struct virtqueue *rxq);
>
> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
> struct rte_mbuf *m);
> #else
> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
> __rte_unused struct rte_mbuf *m) {
> return -1;
> }
> #endif
>
> and remove most #ifdef ... #endif in *.c file.
>
I guess, above approach wont work for non-x86 arch, ad those func are
dummy, right? also code wont build for arm/non-86 arch because
tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx
api.
> Thanks,
> Michael
On 3/1/2016 5:46 PM, Santosh Shukla wrote:
> On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael <michael.qiu@intel.com> wrote:
>> On 2/26/2016 4:53 PM, Santosh Shukla wrote:
>>> Check cpuflag macro before using vectored api.
>>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>>> - Also wrap other vectored freind api ie..
>>> 1) virtqueue_enqueue_recv_refill_simple
>>> 2) virtio_rxq_vec_setup
>>>
>>> todo:
>>> 1) Move virtio_recv_pkts_vec() implementation to
>>> drivers/virtio/virtio_vec_<arch>.h file.
>>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
>>> files to provide vectored/non-vectored rx/tx apis.
>>>
>>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>>> ---
>>> - v1: This is a rework of patch [1].
>>> Note: This patch will let non-x86 arch to use virtio pmd.
>>>
>>> [1] http://dpdk.org/dev/patchwork/patch/10429/
>>>
>>> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
>>> drivers/net/virtio/virtio_rxtx.h | 2 ++
>>> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
>>> 3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>> index 41a1366..ec0b8de 100644
>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>> @@ -67,7 +67,9 @@
>>> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
>>> ETH_TXQ_FLAGS_NOOFFLOADS)
>>>
>>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
>>> static int use_simple_rxtx;
>>> +#endif
>>>
>>>
>> I don't think so much #ifdef ... #endif in *.c file is a good choice.
>> Would you consider let it only in header file like:
>>
>> in drivers/net/virtio/virtio_rxtx.h
>>
>> [...]
>>
>> #ifdef RTE_MACHINE_CPUFLAG_SSSE3
>> int virtio_rxq_vec_setup(struct virtqueue *rxq);
>>
>> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
>> struct rte_mbuf *m);
>> #else
>> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
>> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
>> __rte_unused struct rte_mbuf *m) {
>> return -1;
>> }
>> #endif
>>
>> and remove most #ifdef ... #endif in *.c file.
>>
> I guess, above approach wont work for non-x86 arch, ad those func are
> dummy, right? also code wont build for arm/non-86 arch because
> tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx
> api.
You may right, but you really need to reduce the #ifdef in *.c files.
Thanks,
Michael
>> Thanks,
>> Michael
On Wed, Mar 02, 2016 at 02:10:14AM +0000, Qiu, Michael wrote:
> On 3/1/2016 5:46 PM, Santosh Shukla wrote:
> > On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael <michael.qiu@intel.com> wrote:
> >> On 2/26/2016 4:53 PM, Santosh Shukla wrote:
> >>> Check cpuflag macro before using vectored api.
> >>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> >>> - Also wrap other vectored freind api ie..
> >>> 1) virtqueue_enqueue_recv_refill_simple
> >>> 2) virtio_rxq_vec_setup
> >>>
> >>> todo:
> >>> 1) Move virtio_recv_pkts_vec() implementation to
> >>> drivers/virtio/virtio_vec_<arch>.h file.
> >>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
> >>> files to provide vectored/non-vectored rx/tx apis.
> >>>
> >>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> >>> ---
> >>> - v1: This is a rework of patch [1].
> >>> Note: This patch will let non-x86 arch to use virtio pmd.
> >>>
> >>> [1] http://dpdk.org/dev/patchwork/patch/10429/
> >>>
> >>> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
> >>> drivers/net/virtio/virtio_rxtx.h | 2 ++
> >>> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
> >>> 3 files changed, 27 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> >>> index 41a1366..ec0b8de 100644
> >>> --- a/drivers/net/virtio/virtio_rxtx.c
> >>> +++ b/drivers/net/virtio/virtio_rxtx.c
> >>> @@ -67,7 +67,9 @@
> >>> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
> >>> ETH_TXQ_FLAGS_NOOFFLOADS)
> >>>
> >>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
> >>> static int use_simple_rxtx;
> >>> +#endif
> >>>
> >>>
> >> I don't think so much #ifdef ... #endif in *.c file is a good choice.
> >> Would you consider let it only in header file like:
> >>
> >> in drivers/net/virtio/virtio_rxtx.h
> >>
> >> [...]
> >>
> >> #ifdef RTE_MACHINE_CPUFLAG_SSSE3
> >> int virtio_rxq_vec_setup(struct virtqueue *rxq);
> >>
> >> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
> >> struct rte_mbuf *m);
> >> #else
> >> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
> >> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
> >> __rte_unused struct rte_mbuf *m) {
> >> return -1;
> >> }
> >> #endif
> >>
> >> and remove most #ifdef ... #endif in *.c file.
> >>
> > I guess, above approach wont work for non-x86 arch, ad those func are
> > dummy, right? also code wont build for arm/non-86 arch because
> > tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx
> > api.
>
> You may right, but you really need to reduce the #ifdef in *.c files.
In general, yes. But for this case, no: those vec stuff are for
platforms that support it. For other platforms, we should not
invoke a dummy function like virtio_rxq_vec_setup() at all.
The right way to go is to add another wrapper beyond the vector
stuff, something like:
virtio_rxq_setup()
{
if (has_vec_support)
virtio_rxq_vec_setup();
else
virtio_rxq_generic_setup();
}
Where virtio_rxq_vec_setup() could have a per-arch implementation,
say for X86, or ARM.
It touchs more code, but for now, I'd like to make it simple first.
With the virtio_rxtx_simple.c isolated from Makefile, there aren't
many #ifdef after all.
--yliu
On 3/2/2016 10:48 AM, Yuanhan Liu wrote:
> On Wed, Mar 02, 2016 at 02:10:14AM +0000, Qiu, Michael wrote:
>> On 3/1/2016 5:46 PM, Santosh Shukla wrote:
>>> On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael <michael.qiu@intel.com> wrote:
>>>> On 2/26/2016 4:53 PM, Santosh Shukla wrote:
>>>>> Check cpuflag macro before using vectored api.
>>>>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
>>>>> - Also wrap other vectored freind api ie..
>>>>> 1) virtqueue_enqueue_recv_refill_simple
>>>>> 2) virtio_rxq_vec_setup
>>>>>
>>>>> todo:
>>>>> 1) Move virtio_recv_pkts_vec() implementation to
>>>>> drivers/virtio/virtio_vec_<arch>.h file.
>>>>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
>>>>> files to provide vectored/non-vectored rx/tx apis.
>>>>>
>>>>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
>>>>> ---
>>>>> - v1: This is a rework of patch [1].
>>>>> Note: This patch will let non-x86 arch to use virtio pmd.
>>>>>
>>>>> [1] http://dpdk.org/dev/patchwork/patch/10429/
>>>>>
>>>>> drivers/net/virtio/virtio_rxtx.c | 16 +++++++++++++++-
>>>>> drivers/net/virtio/virtio_rxtx.h | 2 ++
>>>>> drivers/net/virtio/virtio_rxtx_simple.c | 11 ++++++++++-
>>>>> 3 files changed, 27 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>>>> index 41a1366..ec0b8de 100644
>>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>>> @@ -67,7 +67,9 @@
>>>>> #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
>>>>> ETH_TXQ_FLAGS_NOOFFLOADS)
>>>>>
>>>>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
>>>>> static int use_simple_rxtx;
>>>>> +#endif
>>>>>
>>>>>
>>>> I don't think so much #ifdef ... #endif in *.c file is a good choice.
>>>> Would you consider let it only in header file like:
>>>>
>>>> in drivers/net/virtio/virtio_rxtx.h
>>>>
>>>> [...]
>>>>
>>>> #ifdef RTE_MACHINE_CPUFLAG_SSSE3
>>>> int virtio_rxq_vec_setup(struct virtqueue *rxq);
>>>>
>>>> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
>>>> struct rte_mbuf *m);
>>>> #else
>>>> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
>>>> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
>>>> __rte_unused struct rte_mbuf *m) {
>>>> return -1;
>>>> }
>>>> #endif
>>>>
>>>> and remove most #ifdef ... #endif in *.c file.
>>>>
>>> I guess, above approach wont work for non-x86 arch, ad those func are
>>> dummy, right? also code wont build for arm/non-86 arch because
>>> tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx
>>> api.
>> You may right, but you really need to reduce the #ifdef in *.c files.
> In general, yes. But for this case, no: those vec stuff are for
> platforms that support it. For other platforms, we should not
> invoke a dummy function like virtio_rxq_vec_setup() at all.
>
> The right way to go is to add another wrapper beyond the vector
> stuff, something like:
>
> virtio_rxq_setup()
> {
>
> if (has_vec_support)
> virtio_rxq_vec_setup();
> else
> virtio_rxq_generic_setup();
> }
Actually, we could call vec first and if set up failed, fall back to
simple mode. Thus we could use dummy func, and it could make lift simple.
Thanks,
Michael
> Where virtio_rxq_vec_setup() could have a per-arch implementation,
> say for X86, or ARM.
>
> It touchs more code, but for now, I'd like to make it simple first.
> With the virtio_rxtx_simple.c isolated from Makefile, there aren't
> many #ifdef after all.
>
> --yliu
>
@@ -67,7 +67,9 @@
#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
ETH_TXQ_FLAGS_NOOFFLOADS)
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
static int use_simple_rxtx;
+#endif
static void
vq_ring_free_chain(struct virtqueue *vq, uint16_t desc_idx)
@@ -307,12 +309,13 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
nbufs = 0;
error = ENOSPC;
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
if (use_simple_rxtx)
for (i = 0; i < vq->vq_nentries; i++) {
vq->vq_ring.avail->ring[i] = i;
vq->vq_ring.desc[i].flags = VRING_DESC_F_WRITE;
}
-
+#endif
memset(&vq->fake_mbuf, 0, sizeof(vq->fake_mbuf));
for (i = 0; i < RTE_PMD_VIRTIO_RX_MAX_BURST; i++)
vq->sw_ring[vq->vq_nentries + i] = &vq->fake_mbuf;
@@ -325,9 +328,11 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
/******************************************
* Enqueue allocated buffers *
*******************************************/
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
if (use_simple_rxtx)
error = virtqueue_enqueue_recv_refill_simple(vq, m);
else
+#endif
error = virtqueue_enqueue_recv_refill(vq, m);
if (error) {
rte_pktmbuf_free(m);
@@ -340,6 +345,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs);
} else if (queue_type == VTNET_TQ) {
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
if (use_simple_rxtx) {
int mid_idx = vq->vq_nentries >> 1;
for (i = 0; i < mid_idx; i++) {
@@ -357,6 +363,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
for (i = mid_idx; i < vq->vq_nentries; i++)
vq->vq_ring.avail->ring[i] = i;
}
+#endif
}
}
@@ -423,7 +430,9 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
dev->data->rx_queues[queue_idx] = vq;
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
virtio_rxq_vec_setup(vq);
+#endif
return 0;
}
@@ -449,7 +458,10 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
const struct rte_eth_txconf *tx_conf)
{
uint8_t vtpci_queue_idx = 2 * queue_idx + VTNET_SQ_TQ_QUEUE_IDX;
+
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
struct virtio_hw *hw = dev->data->dev_private;
+#endif
struct virtqueue *vq;
uint16_t tx_free_thresh;
int ret;
@@ -462,6 +474,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
return -EINVAL;
}
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
/* Use simple rx/tx func if single segment and no offloads */
if ((tx_conf->txq_flags & VIRTIO_SIMPLE_FLAGS) == VIRTIO_SIMPLE_FLAGS &&
!vtpci_with_feature(hw, VIRTIO_NET_F_MRG_RXBUF)) {
@@ -470,6 +483,7 @@ virtio_dev_tx_queue_setup(struct rte_eth_dev *dev,
dev->rx_pkt_burst = virtio_recv_pkts_vec;
use_simple_rxtx = 1;
}
+#endif
ret = virtio_dev_queue_setup(dev, VTNET_TQ, queue_idx, vtpci_queue_idx,
nb_desc, socket_id, &vq);
@@ -33,7 +33,9 @@
#define RTE_PMD_VIRTIO_RX_MAX_BURST 64
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
int virtio_rxq_vec_setup(struct virtqueue *rxq);
int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
struct rte_mbuf *m);
+#endif
@@ -37,7 +37,9 @@
#include <string.h>
#include <errno.h>
-#include <tmmintrin.h>
+#ifdef __SSE3__
+#include <rte_vect.h>
+#endif
#include <rte_cycles.h>
#include <rte_memory.h>
@@ -66,6 +68,7 @@
#pragma GCC diagnostic ignored "-Wcast-qual"
#endif
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
int __attribute__((cold))
virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
struct rte_mbuf *cookie)
@@ -90,6 +93,7 @@ virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
return 0;
}
+#endif
static inline void
virtio_rxq_rearm_vec(struct virtqueue *rxvq)
@@ -130,6 +134,7 @@ virtio_rxq_rearm_vec(struct virtqueue *rxvq)
vq_update_avail_idx(rxvq);
}
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
/* virtio vPMD receive routine, only accept(nb_pkts >= RTE_VIRTIO_DESC_PER_LOOP)
*
* This routine is for non-mergeable RX, one desc for each guest buffer.
@@ -291,6 +296,7 @@ virtio_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
rxvq->packets += nb_pkts_received;
return nb_pkts_received;
}
+#endif
#define VIRTIO_TX_FREE_THRESH 32
#define VIRTIO_TX_MAX_FREE_BUF_SZ 32
@@ -398,6 +404,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
return nb_pkts;
}
+#ifdef RTE_MACHINE_CPUFLAG_SSSE3
int __attribute__((cold))
virtio_rxq_vec_setup(struct virtqueue *rxq)
{
@@ -416,3 +423,5 @@ virtio_rxq_vec_setup(struct virtqueue *rxq)
return 0;
}
+#endif
+