| Message ID | 20240124074201.8239-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/accel: Use RCU_READ macros | expand |
On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >Replace the manual rcu_read_(un)lock calls by the >*RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba >"docs/style: call out the use of GUARD macros"). > >Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >--- > hw/vfio/common.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > >diff --git a/hw/vfio/common.c b/hw/vfio/common.c >index 4aa86f563c..09878a3603 100644 >--- a/hw/vfio/common.c >+++ b/hw/vfio/common.c >@@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > return; > } > >- rcu_read_lock(); >+ RCU_READ_LOCK_GUARD(); > > if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { > bool read_only; > > if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >- goto out; >+ return; Since this is the only early return, we could alternatively do: - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { + if (vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { remove the goto/return, and wrap the rest of the codeflow in this if's brackets. And then we could use WITH_RCU_READ_LOCK_GUARD instead. That'd increase the code indentation however. > } > /* > * vaddr is only valid until rcu_read_unlock(). But after >@@ -343,8 +343,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > vfio_set_migration_error(ret); > } > } >-out: >- rcu_read_unlock(); > } > > static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl, >@@ -1223,23 +1221,23 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) > if (iotlb->target_as != &address_space_memory) { > error_report("Wrong target AS \"%s\", only system memory is allowed", > iotlb->target_as->name ? iotlb->target_as->name : "none"); >- goto out; >- } >- >- rcu_read_lock(); >- if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { >- ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, >- translated_addr); >- if (ret) { >- error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " >- "0x%"HWADDR_PRIx") = %d (%s)", >- bcontainer, iova, iotlb->addr_mask + 1, ret, >- strerror(-ret)); >+ } else { >+ WITH_RCU_READ_LOCK_GUARD() { >+ if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { >+ ret = vfio_get_dirty_bitmap(bcontainer, iova, >+ iotlb->addr_mask + 1, >+ translated_addr); >+ if (ret) { >+ error_report("vfio_iommu_map_dirty_notify(%p," >+ " 0x%"HWADDR_PRIx >+ ", 0x%"HWADDR_PRIx") = %d (%s)", >+ bcontainer, iova, iotlb->addr_mask + 1, ret, >+ strerror(-ret)); >+ } >+ } > } > } >- rcu_read_unlock(); > >-out: > if (ret) { > vfio_set_migration_error(ret); > } >-- >2.41.0 > Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
On 24/1/24 10:25, Manos Pitsidianakis wrote: > On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> > wrote: >> Replace the manual rcu_read_(un)lock calls by the >> *RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba >> "docs/style: call out the use of GUARD macros"). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/vfio/common.c | 34 ++++++++++++++++------------------ >> 1 file changed, 16 insertions(+), 18 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 4aa86f563c..09878a3603 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier >> *n, IOMMUTLBEntry *iotlb) >> return; >> } >> >> - rcu_read_lock(); >> + RCU_READ_LOCK_GUARD(); >> >> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { >> bool read_only; >> >> if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >> - goto out; >> + return; > > Since this is the only early return, we could alternatively do: > > - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { > + if (vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { > > remove the goto/return, and wrap the rest of the codeflow in this if's > brackets. And then we could use WITH_RCU_READ_LOCK_GUARD instead. That'd > increase the code indentation however. If the maintainer agrees with the style & code churn, I don't mind respining. > >> } >> /* >> * vaddr is only valid until rcu_read_unlock(). But after >> @@ -343,8 +343,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier >> *n, IOMMUTLBEntry *iotlb) >> vfio_set_migration_error(ret); >> } >> } >> -out: >> - rcu_read_unlock(); >> } > Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Thanks!
On 24/1/24 15:09, Philippe Mathieu-Daudé wrote: > On 24/1/24 10:25, Manos Pitsidianakis wrote: >> On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> >> wrote: >>> Replace the manual rcu_read_(un)lock calls by the >>> *RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba >>> "docs/style: call out the use of GUARD macros"). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/vfio/common.c | 34 ++++++++++++++++------------------ >>> 1 file changed, 16 insertions(+), 18 deletions(-) >>> >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 4aa86f563c..09878a3603 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier >>> *n, IOMMUTLBEntry *iotlb) >>> return; >>> } >>> >>> - rcu_read_lock(); >>> + RCU_READ_LOCK_GUARD(); >>> >>> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { >>> bool read_only; >>> >>> if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >>> - goto out; >>> + return; >> >> Since this is the only early return, we could alternatively do: >> >> - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >> + if (vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >> >> remove the goto/return, and wrap the rest of the codeflow in this if's >> brackets. And then we could use WITH_RCU_READ_LOCK_GUARD instead. >> That'd increase the code indentation however. > > If the maintainer agrees with the style & code churn, I don't > mind respining. Alex, Cédric, any preference? > >> >>> } >>> /* >>> * vaddr is only valid until rcu_read_unlock(). But after >>> @@ -343,8 +343,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier >>> *n, IOMMUTLBEntry *iotlb) >>> vfio_set_migration_error(ret); >>> } >>> } >>> -out: >>> - rcu_read_unlock(); >>> } > >> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > > Thanks!
On 2/16/24 09:49, Philippe Mathieu-Daudé wrote: > On 24/1/24 15:09, Philippe Mathieu-Daudé wrote: >> On 24/1/24 10:25, Manos Pitsidianakis wrote: >>> On Wed, 24 Jan 2024 09:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >>>> Replace the manual rcu_read_(un)lock calls by the >>>> *RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba >>>> "docs/style: call out the use of GUARD macros"). >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> --- >>>> hw/vfio/common.c | 34 ++++++++++++++++------------------ >>>> 1 file changed, 16 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>>> index 4aa86f563c..09878a3603 100644 >>>> --- a/hw/vfio/common.c >>>> +++ b/hw/vfio/common.c >>>> @@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) >>>> return; >>>> } >>>> >>>> - rcu_read_lock(); >>>> + RCU_READ_LOCK_GUARD(); >>>> >>>> if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { >>>> bool read_only; >>>> >>>> if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >>>> - goto out; >>>> + return; >>> >>> Since this is the only early return, we could alternatively do: >>> >>> - if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >>> + if (vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { >>> >>> remove the goto/return, and wrap the rest of the codeflow in this if's brackets. And then we could use WITH_RCU_READ_LOCK_GUARD instead. That'd increase the code indentation however. >> >> If the maintainer agrees with the style & code churn, I don't >> mind respining. > > Alex, Cédric, any preference? my choice would be to keep the 'goto' statement and protect the vfio_get_xlat_addr() call with : + WITH_RCU_READ_LOCK_GUARD() { + if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { + ret = vfio_get_dirty_bitmap(bcontainer, iova, + iotlb->addr_mask + 1, + translated_addr); + if (ret) { + error_report("vfio_iommu_map_dirty_notify(%p," + " 0x%"HWADDR_PRIx + ", 0x%"HWADDR_PRIx") = %d (%s)", + bcontainer, iova, iotlb->addr_mask + 1, ret, + strerror(-ret)); + } + } } Thanks, C.
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 4aa86f563c..09878a3603 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -308,13 +308,13 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) return; } - rcu_read_lock(); + RCU_READ_LOCK_GUARD(); if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) { bool read_only; if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) { - goto out; + return; } /* * vaddr is only valid until rcu_read_unlock(). But after @@ -343,8 +343,6 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) vfio_set_migration_error(ret); } } -out: - rcu_read_unlock(); } static void vfio_ram_discard_notify_discard(RamDiscardListener *rdl, @@ -1223,23 +1221,23 @@ static void vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb) if (iotlb->target_as != &address_space_memory) { error_report("Wrong target AS \"%s\", only system memory is allowed", iotlb->target_as->name ? iotlb->target_as->name : "none"); - goto out; - } - - rcu_read_lock(); - if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { - ret = vfio_get_dirty_bitmap(bcontainer, iova, iotlb->addr_mask + 1, - translated_addr); - if (ret) { - error_report("vfio_iommu_map_dirty_notify(%p, 0x%"HWADDR_PRIx", " - "0x%"HWADDR_PRIx") = %d (%s)", - bcontainer, iova, iotlb->addr_mask + 1, ret, - strerror(-ret)); + } else { + WITH_RCU_READ_LOCK_GUARD() { + if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) { + ret = vfio_get_dirty_bitmap(bcontainer, iova, + iotlb->addr_mask + 1, + translated_addr); + if (ret) { + error_report("vfio_iommu_map_dirty_notify(%p," + " 0x%"HWADDR_PRIx + ", 0x%"HWADDR_PRIx") = %d (%s)", + bcontainer, iova, iotlb->addr_mask + 1, ret, + strerror(-ret)); + } + } } } - rcu_read_unlock(); -out: if (ret) { vfio_set_migration_error(ret); }
Replace the manual rcu_read_(un)lock calls by the *RCU_READ_LOCK_GUARD macros (See commit ef46ae67ba "docs/style: call out the use of GUARD macros"). Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/vfio/common.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)