| Message ID | 20220422134529.1775379-1-harry.van.haaren@intel.com |
|---|---|
| State | Accepted |
| Commit | 5db8aa39d9b7f6317f0d450e503de53865fa5593 |
| Headers | show |
| Series | [ovs-dev,v2] dpif-netdev-avx512: fix ubsan shift error in bitmasks | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 22 Apr 2022, at 15:45, Harry van Haaren wrote: > The code changes here are to handle (1 << i) shifts where 'i' is the > packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > > Fixed by adding UINT32_C() around the 1 character, ensuring compiler knows > the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer is now happy > with the bit-shifts at runtime. > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- Compile tested as I do not have an AVX machine. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 4/22/22 15:45, Harry van Haaren wrote: > The code changes here are to handle (1 << i) shifts where 'i' is the > packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > > Fixed by adding UINT32_C() around the 1 character, ensuring compiler knows > the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer is now happy > with the bit-shifts at runtime. > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- > > v2: > - Suggested improvements to change 1ULL to UINT32_C(1) (David, Eelco) > - Squashed the MFEX avx512 fixup into this patch > > Thanks Ilya for the detail in the email - reworked as commit message; > https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html > > --- > lib/dpif-netdev-avx512.c | 10 +++++----- > lib/dpif-netdev-extract-avx512.c | 2 +- > 2 files changed, 6 insertions(+), 6 deletions(-) Hey, Michael. Is Intel CI alive? I can't find a report for this patch. It seems that the last report was sent about a week ago. Best regards, Ilya Maximets.
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Tuesday 26 April 2022 10:02 > To: ovs-dev@openvswitch.org; Phelan, Michael > <michael.phelan@intel.com> > Cc: i.maximets@ovn.org; echaudro@redhat.com; > david.marchand@redhat.com; Van Haaren, Harry > <harry.van.haaren@intel.com>; Aaron Conole <aconole@redhat.com>; > Stokes, Ian <ian.stokes@intel.com> > Subject: Re: [PATCH v2] dpif-netdev-avx512: fix ubsan shift error in bitmasks > > On 4/22/22 15:45, Harry van Haaren wrote: > > The code changes here are to handle (1 << i) shifts where 'i' is the > > packet index in the batch, and 1 << 31 is an overflow of the signed '1'. > > > > Fixed by adding UINT32_C() around the 1 character, ensuring compiler > > knows the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer > > is now happy with the bit-shifts at runtime. > > > > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > --- > > > > v2: > > - Suggested improvements to change 1ULL to UINT32_C(1) (David, Eelco) > > - Squashed the MFEX avx512 fixup into this patch > > > > Thanks Ilya for the detail in the email - reworked as commit message; > > https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html > > > > --- > > lib/dpif-netdev-avx512.c | 10 +++++----- > > lib/dpif-netdev-extract-avx512.c | 2 +- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > Hey, Michael. > > Is Intel CI alive? I can't find a report for this patch. > It seems that the last report was sent about a week ago. Hey Ilya, Yes the CI is still running, the account credentials were not up to date on Jenkins so the emails were not being sent. I've updated them now and will rerun the tests from the last week or so that are missing report emails. Thanks, Michael. > > Best regards, Ilya Maximets.
On 4/26/22 11:59, Phelan, Michael wrote: > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Tuesday 26 April 2022 10:02 >> To: ovs-dev@openvswitch.org; Phelan, Michael >> <michael.phelan@intel.com> >> Cc: i.maximets@ovn.org; echaudro@redhat.com; >> david.marchand@redhat.com; Van Haaren, Harry >> <harry.van.haaren@intel.com>; Aaron Conole <aconole@redhat.com>; >> Stokes, Ian <ian.stokes@intel.com> >> Subject: Re: [PATCH v2] dpif-netdev-avx512: fix ubsan shift error in bitmasks >> >> On 4/22/22 15:45, Harry van Haaren wrote: >>> The code changes here are to handle (1 << i) shifts where 'i' is the >>> packet index in the batch, and 1 << 31 is an overflow of the signed '1'. >>> >>> Fixed by adding UINT32_C() around the 1 character, ensuring compiler >>> knows the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer >>> is now happy with the bit-shifts at runtime. >>> >>> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >>> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >>> >>> --- >>> >>> v2: >>> - Suggested improvements to change 1ULL to UINT32_C(1) (David, Eelco) >>> - Squashed the MFEX avx512 fixup into this patch >>> >>> Thanks Ilya for the detail in the email - reworked as commit message; >>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html >>> >>> --- >>> lib/dpif-netdev-avx512.c | 10 +++++----- >>> lib/dpif-netdev-extract-avx512.c | 2 +- >>> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> Hey, Michael. >> >> Is Intel CI alive? I can't find a report for this patch. >> It seems that the last report was sent about a week ago. > Hey Ilya, > > Yes the CI is still running, the account credentials were not up to date on Jenkins so the emails were not being sent. > I've updated them now and will rerun the tests from the last week or so that are missing report emails. OK. Thanks! > > Thanks, > Michael. > > >> >> Best regards, Ilya Maximets.
On 4/22/22 17:13, Eelco Chaudron wrote: > > > On 22 Apr 2022, at 15:45, Harry van Haaren wrote: > >> The code changes here are to handle (1 << i) shifts where 'i' is the >> packet index in the batch, and 1 << 31 is an overflow of the signed '1'. >> >> Fixed by adding UINT32_C() around the 1 character, ensuring compiler knows >> the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer is now happy >> with the bit-shifts at runtime. >> >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> >> >> --- > > Compile tested as I do not have an AVX machine. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Thanks! Applied to master and branch-2.17. Best regards, Ilya Maximets.
diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index b7131ba3f..151a945a9 100644 --- a/lib/dpif-netdev-avx512.c +++ b/lib/dpif-netdev-avx512.c @@ -159,7 +159,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, mf_mask = mfex_func(packets, keys, batch_size, in_port, pmd); } - uint32_t lookup_pkts_bitmask = (1ULL << batch_size) - 1; + uint32_t lookup_pkts_bitmask = (UINT32_C(1) << batch_size) - 1; uint32_t iter = lookup_pkts_bitmask; while (iter) { uint32_t i = raw_ctz(iter); @@ -183,7 +183,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, * classifed by vector mfex else do a scalar miniflow extract * for that packet. */ - bool mfex_hit = !!(mf_mask & (1 << i)); + bool mfex_hit = !!(mf_mask & (UINT32_C(1) << i)); /* Check for a partial hardware offload match. */ if (hwol_enabled) { @@ -204,7 +204,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, pkt_meta[i].bytes = dp_packet_size(packet); phwol_hits++; - hwol_emc_smc_hitmask |= (1 << i); + hwol_emc_smc_hitmask |= (UINT32_C(1) << i); continue; } } @@ -227,7 +227,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, if (f) { rules[i] = &f->cr; emc_hits++; - hwol_emc_smc_hitmask |= (1 << i); + hwol_emc_smc_hitmask |= (UINT32_C(1) << i); continue; } } @@ -237,7 +237,7 @@ dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd, if (f) { rules[i] = &f->cr; smc_hits++; - smc_hitmask |= (1 << i); + smc_hitmask |= (UINT32_C(1) << i); continue; } } diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c index c1c1fefb6..a0fedb137 100644 --- a/lib/dpif-netdev-extract-avx512.c +++ b/lib/dpif-netdev-extract-avx512.c @@ -619,7 +619,7 @@ mfex_avx512_process(struct dp_packet_batch *packets, }; /* This packet has its miniflow created, add to hitmask. */ - hitmask |= 1 << i; + hitmask |= UINT32_C(1) << i; } return hitmask;
The code changes here are to handle (1 << i) shifts where 'i' is the packet index in the batch, and 1 << 31 is an overflow of the signed '1'. Fixed by adding UINT32_C() around the 1 character, ensuring compiler knows the 1 is unsigned (and 32-bits). Undefined Behaviour sanitizer is now happy with the bit-shifts at runtime. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> --- v2: - Suggested improvements to change 1ULL to UINT32_C(1) (David, Eelco) - Squashed the MFEX avx512 fixup into this patch Thanks Ilya for the detail in the email - reworked as commit message; https://mail.openvswitch.org/pipermail/ovs-dev/2022-April/393270.html --- lib/dpif-netdev-avx512.c | 10 +++++----- lib/dpif-netdev-extract-avx512.c | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-)