| Message ID | 20191113170104.44648-1-malvika.gupta@arm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dpif-netdev-perf: aarch64 support for accurate timing update of TSC cycle counter | expand |
On 13.11.2019 18:01, Malvika Gupta wrote: > The accurate timing implementation in this patch gets the wall clock counter via > cntvct_el0 register access. This call is portable to all aarch64 architectures > and has been verified on an 64-bit arm server. > > Suggested-by: Yanqin Wei <yanqin.wei@arm.com> > Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> > --- Thanks for the patch! Are you trying to use AF_XDP on aarch64? Asking because it's the only real scenario where this patch can be useful. For the patch subject, I'd suggest to shorten it a little. 'timing', 'TSC' and 'cycle counter' are kind of synonyms here and doesn't make the sentence any clear. Suggesting something like this: "dpif-netdev-perf: Accurate cycle counter update on aarch64." What do you think? One more comment inline. > lib/dpif-netdev-perf.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h > index ce369375b..4ea7cc355 100644 > --- a/lib/dpif-netdev-perf.h > +++ b/lib/dpif-netdev-perf.h > @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats *s) > asm volatile("rdtsc" : "=a" (l), "=d" (h)); > > return s->last_tsc = ((uint64_t) h << 32) | l; > +#elif !defined(_MSC_VER) && defined(__aarch64__) > + uint64_t tsc; > + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); > + > + return s->last_tsc = tsc; I think we could drop the 'tsc' local variable here and write directly to s->last_tsc. Less number of variables and operations. Best regards, Ilya Maximets.
Hi Ilya, No, we didn't test this patch based on OVS-AF_XDP, but made a black build to enable this in OVS-DPDK and test it. Currently DPDK-AF_XDP has been tested in latest kernel (not released). So I think OVS-AF_XDP is close to be supported for aarch64. Furthermore, I found a document about userspace-only mode of Open vSwitch without DPDK. http://docs.openvswitch.org/en/latest/intro/install/userspace/#using-the-userspace-datapath-with-ovs-vswitchd So it seems userspace datapath should be decoupled with networking IO, users can even customize this. Does it means we need implement all used DPDK API inside OVS? Best Regards, Wei Yanqin > -----Original Message----- > From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Ilya Maximets > Sent: Tuesday, November 26, 2019 11:38 PM > To: Malvika Gupta <Malvika.Gupta@arm.com>; dev@openvswitch.org > Cc: nd <nd@arm.com>; Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> > Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for accurate > timing update of TSC cycle counter > > On 13.11.2019 18:01, Malvika Gupta wrote: > > The accurate timing implementation in this patch gets the wall clock > > counter via > > cntvct_el0 register access. This call is portable to all aarch64 > > architectures and has been verified on an 64-bit arm server. > > > > Suggested-by: Yanqin Wei <yanqin.wei@arm.com> > > Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> > > --- > > Thanks for the patch! > > Are you trying to use AF_XDP on aarch64? Asking because it's the only real > scenario where this patch can be useful. > > For the patch subject, I'd suggest to shorten it a little. > 'timing', 'TSC' and 'cycle counter' are kind of synonyms here and doesn't make > the sentence any clear. Suggesting something like this: > "dpif-netdev-perf: Accurate cycle counter update on aarch64." > > What do you think? > > One more comment inline. > > > lib/dpif-netdev-perf.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h index > > ce369375b..4ea7cc355 100644 > > --- a/lib/dpif-netdev-perf.h > > +++ b/lib/dpif-netdev-perf.h > > @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats *s) > > asm volatile("rdtsc" : "=a" (l), "=d" (h)); > > > > return s->last_tsc = ((uint64_t) h << 32) | l; > > +#elif !defined(_MSC_VER) && defined(__aarch64__) > > + uint64_t tsc; > > + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); > > + > > + return s->last_tsc = tsc; > > I think we could drop the 'tsc' local variable here and write directly to s- > >last_tsc. Less number of variables and operations. > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 27.11.2019 8:38, Yanqin Wei (Arm Technology China) wrote: > Hi Ilya, > > No, we didn't test this patch based on OVS-AF_XDP, but made a black build to enable this in OVS-DPDK and test it. > Currently DPDK-AF_XDP has been tested in latest kernel (not released). So I think OVS-AF_XDP is close to be supported for aarch64. > > Furthermore, I found a document about userspace-only mode of Open vSwitch without DPDK. > http://docs.openvswitch.org/en/latest/intro/install/userspace/#using-the-userspace-datapath-with-ovs-vswitchd > So it seems userspace datapath should be decoupled with networking IO, users can even customize this. Does it means we need implement all used DPDK API inside OVS? Userspace datapath in OVS doesn't depend on DPDK. DPDK only provides a few port types (dpdk, dpdkvhostuser, etc.). It's fully functional by itself. For example you may use it to forward packets with OVS-native afxdp ports: http://docs.openvswitch.org/en/latest/intro/install/afxdp/ Best regards, Ilya Maximets. > > Best Regards, > Wei Yanqin > > >> -----Original Message----- >> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Ilya Maximets >> Sent: Tuesday, November 26, 2019 11:38 PM >> To: Malvika Gupta <Malvika.Gupta@arm.com>; dev@openvswitch.org >> Cc: nd <nd@arm.com>; Honnappa Nagarahalli >> <Honnappa.Nagarahalli@arm.com> >> Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for accurate >> timing update of TSC cycle counter >> >> On 13.11.2019 18:01, Malvika Gupta wrote: >>> The accurate timing implementation in this patch gets the wall clock >>> counter via >>> cntvct_el0 register access. This call is portable to all aarch64 >>> architectures and has been verified on an 64-bit arm server. >>> >>> Suggested-by: Yanqin Wei <yanqin.wei@arm.com> >>> Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> >>> --- >> >> Thanks for the patch! >> >> Are you trying to use AF_XDP on aarch64? Asking because it's the only real >> scenario where this patch can be useful. >> >> For the patch subject, I'd suggest to shorten it a little. >> 'timing', 'TSC' and 'cycle counter' are kind of synonyms here and doesn't make >> the sentence any clear. Suggesting something like this: >> "dpif-netdev-perf: Accurate cycle counter update on aarch64." >> >> What do you think? >> >> One more comment inline. >> >>> lib/dpif-netdev-perf.h | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h index >>> ce369375b..4ea7cc355 100644 >>> --- a/lib/dpif-netdev-perf.h >>> +++ b/lib/dpif-netdev-perf.h >>> @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats *s) >>> asm volatile("rdtsc" : "=a" (l), "=d" (h)); >>> >>> return s->last_tsc = ((uint64_t) h << 32) | l; >>> +#elif !defined(_MSC_VER) && defined(__aarch64__) >>> + uint64_t tsc; >>> + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); >>> + >>> + return s->last_tsc = tsc; >> >> I think we could drop the 'tsc' local variable here and write directly to s- >>> last_tsc. Less number of variables and operations. >> >> Best regards, Ilya Maximets. >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ilya, Please see the inline comments below. Thanks! > -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, November 29, 2019 12:30 AM > To: Yanqin Wei (Arm Technology China) <Yanqin.Wei@arm.com>; Ilya > Maximets <i.maximets@ovn.org>; Malvika Gupta > <Malvika.Gupta@arm.com>; dev@openvswitch.org > Cc: nd <nd@arm.com>; Honnappa Nagarahalli > <Honnappa.Nagarahalli@arm.com> > Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for > accurate timing update of TSC cycle counter > > On 27.11.2019 8:38, Yanqin Wei (Arm Technology China) wrote: > > Hi Ilya, > > > > No, we didn't test this patch based on OVS-AF_XDP, but made a black build > to enable this in OVS-DPDK and test it. > > Currently DPDK-AF_XDP has been tested in latest kernel (not released). So I > think OVS-AF_XDP is close to be supported for aarch64. > > > > Furthermore, I found a document about userspace-only mode of Open > vSwitch without DPDK. > > http://docs.openvswitch.org/en/latest/intro/install/userspace/#using-t > > he-userspace-datapath-with-ovs-vswitchd > > So it seems userspace datapath should be decoupled with networking IO, > users can even customize this. Does it means we need implement all used > DPDK API inside OVS? > > Userspace datapath in OVS doesn't depend on DPDK. DPDK only provides a > few port types (dpdk, dpdkvhostuser, etc.). It's fully functional by itself. For > example you may use it to forward packets with OVS-native afxdp ports: > http://docs.openvswitch.org/en/latest/intro/install/afxdp/ > > Best regards, Ilya Maximets. > How do you suggest we proceed with this patch till OVS-AF_XDP is supported for aarch64? > > > > Best Regards, > > Wei Yanqin > > > > > >> -----Original Message----- > >> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Ilya > >> Maximets > >> Sent: Tuesday, November 26, 2019 11:38 PM > >> To: Malvika Gupta <Malvika.Gupta@arm.com>; dev@openvswitch.org > >> Cc: nd <nd@arm.com>; Honnappa Nagarahalli > >> <Honnappa.Nagarahalli@arm.com> > >> Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for > >> accurate timing update of TSC cycle counter > >> > >> On 13.11.2019 18:01, Malvika Gupta wrote: > >>> The accurate timing implementation in this patch gets the wall clock > >>> counter via > >>> cntvct_el0 register access. This call is portable to all aarch64 > >>> architectures and has been verified on an 64-bit arm server. > >>> > >>> Suggested-by: Yanqin Wei <yanqin.wei@arm.com> > >>> Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> > >>> --- > >> > >> Thanks for the patch! > >> > >> Are you trying to use AF_XDP on aarch64? Asking because it's the > >> only real scenario where this patch can be useful. > >> > >> For the patch subject, I'd suggest to shorten it a little. > >> 'timing', 'TSC' and 'cycle counter' are kind of synonyms here and > >> doesn't make the sentence any clear. Suggesting something like this: > >> "dpif-netdev-perf: Accurate cycle counter update on aarch64." > >> > >> What do you think? I agree we can shorten the name; I will submit the changes in v2. > >> > >> One more comment inline. > >> > >>> lib/dpif-netdev-perf.h | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>> diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h index > >>> ce369375b..4ea7cc355 100644 > >>> --- a/lib/dpif-netdev-perf.h > >>> +++ b/lib/dpif-netdev-perf.h > >>> @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats > *s) > >>> asm volatile("rdtsc" : "=a" (l), "=d" (h)); > >>> > >>> return s->last_tsc = ((uint64_t) h << 32) | l; > >>> +#elif !defined(_MSC_VER) && defined(__aarch64__) > >>> + uint64_t tsc; > >>> + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); > >>> + > >>> + return s->last_tsc = tsc; > >> > >> I think we could drop the 'tsc' local variable here and write > >> directly to s- > >>> last_tsc. Less number of variables and operations. > >> Okay, I will make this change in v2. Best, Malvika > >> Best regards, Ilya Maximets. > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On 03.12.2019 15:42, Malvika Gupta wrote: > Hi Ilya, > > Please see the inline comments below. Thanks! > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Friday, November 29, 2019 12:30 AM >> To: Yanqin Wei (Arm Technology China) <Yanqin.Wei@arm.com>; Ilya >> Maximets <i.maximets@ovn.org>; Malvika Gupta >> <Malvika.Gupta@arm.com>; dev@openvswitch.org >> Cc: nd <nd@arm.com>; Honnappa Nagarahalli >> <Honnappa.Nagarahalli@arm.com> >> Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for >> accurate timing update of TSC cycle counter >> >> On 27.11.2019 8:38, Yanqin Wei (Arm Technology China) wrote: >>> Hi Ilya, >>> >>> No, we didn't test this patch based on OVS-AF_XDP, but made a black build >> to enable this in OVS-DPDK and test it. >>> Currently DPDK-AF_XDP has been tested in latest kernel (not released). So I >> think OVS-AF_XDP is close to be supported for aarch64. >>> >>> Furthermore, I found a document about userspace-only mode of Open >> vSwitch without DPDK. >>> http://docs.openvswitch.org/en/latest/intro/install/userspace/#using-t >>> he-userspace-datapath-with-ovs-vswitchd >>> So it seems userspace datapath should be decoupled with networking IO, >> users can even customize this. Does it means we need implement all used >> DPDK API inside OVS? >> >> Userspace datapath in OVS doesn't depend on DPDK. DPDK only provides a >> few port types (dpdk, dpdkvhostuser, etc.). It's fully functional by itself. For >> example you may use it to forward packets with OVS-native afxdp ports: >> http://docs.openvswitch.org/en/latest/intro/install/afxdp/ >> >> Best regards, Ilya Maximets. >> > > How do you suggest we proceed with this patch till OVS-AF_XDP is supported for aarch64? There is nothing to do from the OVS side, so it's totally OK. The code is working and reachable with a userspace datapath, so we could accept it. Looking forward for v2. Best regards, Ilya Maximets. > >>> >>> Best Regards, >>> Wei Yanqin >>> >>> >>>> -----Original Message----- >>>> From: dev <ovs-dev-bounces@openvswitch.org> On Behalf Of Ilya >>>> Maximets >>>> Sent: Tuesday, November 26, 2019 11:38 PM >>>> To: Malvika Gupta <Malvika.Gupta@arm.com>; dev@openvswitch.org >>>> Cc: nd <nd@arm.com>; Honnappa Nagarahalli >>>> <Honnappa.Nagarahalli@arm.com> >>>> Subject: Re: [ovs-dev] [PATCH] dpif-netdev-perf: aarch64 support for >>>> accurate timing update of TSC cycle counter >>>> >>>> On 13.11.2019 18:01, Malvika Gupta wrote: >>>>> The accurate timing implementation in this patch gets the wall clock >>>>> counter via >>>>> cntvct_el0 register access. This call is portable to all aarch64 >>>>> architectures and has been verified on an 64-bit arm server. >>>>> >>>>> Suggested-by: Yanqin Wei <yanqin.wei@arm.com> >>>>> Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> >>>>> --- >>>> >>>> Thanks for the patch! >>>> >>>> Are you trying to use AF_XDP on aarch64? Asking because it's the >>>> only real scenario where this patch can be useful. >>>> >>>> For the patch subject, I'd suggest to shorten it a little. >>>> 'timing', 'TSC' and 'cycle counter' are kind of synonyms here and >>>> doesn't make the sentence any clear. Suggesting something like this: >>>> "dpif-netdev-perf: Accurate cycle counter update on aarch64." >>>> >>>> What do you think? > > I agree we can shorten the name; I will submit the changes in v2. > >>>> >>>> One more comment inline. >>>> >>>>> lib/dpif-netdev-perf.h | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h index >>>>> ce369375b..4ea7cc355 100644 >>>>> --- a/lib/dpif-netdev-perf.h >>>>> +++ b/lib/dpif-netdev-perf.h >>>>> @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats >> *s) >>>>> asm volatile("rdtsc" : "=a" (l), "=d" (h)); >>>>> >>>>> return s->last_tsc = ((uint64_t) h << 32) | l; >>>>> +#elif !defined(_MSC_VER) && defined(__aarch64__) >>>>> + uint64_t tsc; >>>>> + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); >>>>> + >>>>> + return s->last_tsc = tsc; >>>> >>>> I think we could drop the 'tsc' local variable here and write >>>> directly to s- >>>>> last_tsc. Less number of variables and operations. >>>> > > Okay, I will make this change in v2. > Best, > Malvika > >>>> Best regards, Ilya Maximets. >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. >
diff --git a/lib/dpif-netdev-perf.h b/lib/dpif-netdev-perf.h index ce369375b..4ea7cc355 100644 --- a/lib/dpif-netdev-perf.h +++ b/lib/dpif-netdev-perf.h @@ -220,6 +220,11 @@ cycles_counter_update(struct pmd_perf_stats *s) asm volatile("rdtsc" : "=a" (l), "=d" (h)); return s->last_tsc = ((uint64_t) h << 32) | l; +#elif !defined(_MSC_VER) && defined(__aarch64__) + uint64_t tsc; + asm volatile("mrs %0, cntvct_el0" : "=r" (tsc)); + + return s->last_tsc = tsc; #elif defined(__linux__) return rdtsc_syscall(s); #else
The accurate timing implementation in this patch gets the wall clock counter via cntvct_el0 register access. This call is portable to all aarch64 architectures and has been verified on an 64-bit arm server. Suggested-by: Yanqin Wei <yanqin.wei@arm.com> Signed-off-by: Malvika Gupta <malvika.gupta@arm.com> --- lib/dpif-netdev-perf.h | 5 +++++ 1 file changed, 5 insertions(+)