Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
On Fri, 2020-08-28 at 11:58 +1000, Alexey Kardashevskiy wrote:
>
> On 28/08/2020 08:11, Leonardo Bras wrote:
> > On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
> > > > static int find_existing_ddw_windows(void)
> > > > {
> > > > int len;
> > > > @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
> > > > if (!direct64)
> > > > continue;
> > > >
> > > > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > > > - if (!window || len < sizeof(struct
> > > > dynamic_dma_window_prop)) {
> > > > + window = ddw_list_add(pdn, direct64);
> > > > + if (!window || len < sizeof(*direct64)) {
> > >
> > > Since you are touching this code, it looks like the "len <
> > > sizeof(*direct64)" part should go above to "if (!direct64)".
> >
> > Sure, makes sense.
> > It will be fixed for v2.
> >
> > >
> > >
> > > > kfree(window);
> > > > remove_ddw(pdn, true);
> > > > - continue;
> > > > }
> > > > -
> > > > - window->device = pdn;
> > > > - window->prop = direct64;
> > > > - spin_lock(_window_list_lock);
> > > > - list_add(>list, _window_list);
> > > > - spin_unlock(_window_list_lock);
> > > > }
> > > >
> > > > return 0;
> > > > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> > > > device_node *pdn)
> > > > dev_dbg(>dev, "created tce table LIOBN 0x%x for %pOF\n",
> > > > create.liobn, dn);
> > > >
> > > > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > > > + /* Add new window to existing DDW list */
> > >
> > > The comment seems to duplicate what the ddw_list_add name already
> > > suggests.
> >
> > Ok, I will remove it then.
> >
> > > > + window = ddw_list_add(pdn, ddwprop);
> > > > if (!window)
> > > > goto out_clear_window;
> > > >
> > > > @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev,
> > > > struct device_node *pdn)
> > > > goto out_free_window;
> > > > }
> > > >
> > > > - window->device = pdn;
> > > > - window->prop = ddwprop;
> > > > - spin_lock(_window_list_lock);
> > > > - list_add(>list, _window_list);
> > > > - spin_unlock(_window_list_lock);
> > >
> > > I'd leave these 3 lines here and in find_existing_ddw_windows() (which
> > > would make ddw_list_add -> ddw_prop_alloc). In general you want to have
> > > less stuff to do on the failure path. kmalloc may fail and needs kfree
> > > but you can safely delay list_add (which cannot fail) and avoid having
> > > the lock help twice in the same function (one of them is hidden inside
> > > ddw_list_add).
> > > Not sure if this change is really needed after all. Thanks,
> >
> > I understand this leads to better performance in case anything fails.
> > Also, I think list_add happening in the end is less error-prone (in
> > case the list is checked between list_add and a fail).
>
> Performance was not in my mind at all.
>
> I noticed you remove from a list with a lock help and it was not there
> before and there is a bunch on labels on the exit path and started
> looking for list_add() and if you do not double remove from the list.
>
>
> > But what if we put it at the end?
> > What is the chance of a kzalloc of 4 pointers (struct direct_window)
> > failing after walk_system_ram_range?
>
> This is not about chances really, it is about readability. If let's say
> kmalloc failed, you just to the error exit label and simply call kfree()
> on that pointer, kfree will do nothing if it is NULL already, simple.
> list_del() does not have this simplicity.
>
>
> > Is it not worthy doing that for making enable_ddw() easier to
> > understand?
>
> This is my goal here :)
Ok, it makes sense to me now.
I tried creating list_add() to keep everything related to list-adding
into a single place, instead of splitting it around the other stuff,
but now I understand that the code may look more complex than it was
before, because of the failing path increasing in size.
For me it was strange creating a list entry end not list_add()ing it
right away, but maybe it's something worth to get used to, as it may
increase the failing path simplicity, since list_add() don't fail.
I will try to see if the ddw_list_add() routine would become a useful
ddw_list_entry(), but if not, I will remove this patch.
Alexey, Thank you for reviewing this series!
Best regards,
Leonardo
Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
On 28/08/2020 08:11, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
>>> static int find_existing_ddw_windows(void)
>>> {
>>> int len;
>>> @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
>>> if (!direct64)
>>> continue;
>>>
>>> - window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
>>> + window = ddw_list_add(pdn, direct64);
>>> + if (!window || len < sizeof(*direct64)) {
>>
>> Since you are touching this code, it looks like the "len <
>> sizeof(*direct64)" part should go above to "if (!direct64)".
>
> Sure, makes sense.
> It will be fixed for v2.
>
>>
>>
>>
>>> kfree(window);
>>> remove_ddw(pdn, true);
>>> - continue;
>>> }
>>> -
>>> - window->device = pdn;
>>> - window->prop = direct64;
>>> - spin_lock(_window_list_lock);
>>> - list_add(>list, _window_list);
>>> - spin_unlock(_window_list_lock);
>>> }
>>>
>>> return 0;
>>> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
>>> device_node *pdn)
>>> dev_dbg(>dev, "created tce table LIOBN 0x%x for %pOF\n",
>>> create.liobn, dn);
>>>
>>> - window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> + /* Add new window to existing DDW list */
>>
>> The comment seems to duplicate what the ddw_list_add name already suggests.
>
> Ok, I will remove it then.
>
>>> + window = ddw_list_add(pdn, ddwprop);
>>> if (!window)
>>> goto out_clear_window;
>>>
>>> @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct
>>> device_node *pdn)
>>> goto out_free_window;
>>> }
>>>
>>> - window->device = pdn;
>>> - window->prop = ddwprop;
>>> - spin_lock(_window_list_lock);
>>> - list_add(>list, _window_list);
>>> - spin_unlock(_window_list_lock);
>>
>> I'd leave these 3 lines here and in find_existing_ddw_windows() (which
>> would make ddw_list_add -> ddw_prop_alloc). In general you want to have
>> less stuff to do on the failure path. kmalloc may fail and needs kfree
>> but you can safely delay list_add (which cannot fail) and avoid having
>> the lock help twice in the same function (one of them is hidden inside
>> ddw_list_add).
>> Not sure if this change is really needed after all. Thanks,
>
> I understand this leads to better performance in case anything fails.
> Also, I think list_add happening in the end is less error-prone (in
> case the list is checked between list_add and a fail).
Performance was not in my mind at all.
I noticed you remove from a list with a lock help and it was not there
before and there is a bunch on labels on the exit path and started
looking for list_add() and if you do not double remove from the list.
> But what if we put it at the end?
> What is the chance of a kzalloc of 4 pointers (struct direct_window)
> failing after walk_system_ram_range?
This is not about chances really, it is about readability. If let's say
kmalloc failed, you just to the error exit label and simply call kfree()
on that pointer, kfree will do nothing if it is NULL already, simple.
list_del() does not have this simplicity.
> Is it not worthy doing that for making enable_ddw() easier to
> understand?
This is my goal here :)
>
> Best regards,
> Leonardo
>
--
Alexey
Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
> > static int find_existing_ddw_windows(void)
> > {
> > int len;
> > @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
> > if (!direct64)
> > continue;
> >
> > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
> > + window = ddw_list_add(pdn, direct64);
> > + if (!window || len < sizeof(*direct64)) {
>
> Since you are touching this code, it looks like the "len <
> sizeof(*direct64)" part should go above to "if (!direct64)".
Sure, makes sense.
It will be fixed for v2.
>
>
>
> > kfree(window);
> > remove_ddw(pdn, true);
> > - continue;
> > }
> > -
> > - window->device = pdn;
> > - window->prop = direct64;
> > - spin_lock(_window_list_lock);
> > - list_add(>list, _window_list);
> > - spin_unlock(_window_list_lock);
> > }
> >
> > return 0;
> > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> > device_node *pdn)
> > dev_dbg(>dev, "created tce table LIOBN 0x%x for %pOF\n",
> > create.liobn, dn);
> >
> > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > + /* Add new window to existing DDW list */
>
> The comment seems to duplicate what the ddw_list_add name already suggests.
Ok, I will remove it then.
> > + window = ddw_list_add(pdn, ddwprop);
> > if (!window)
> > goto out_clear_window;
> >
> > @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> > device_node *pdn)
> > goto out_free_window;
> > }
> >
> > - window->device = pdn;
> > - window->prop = ddwprop;
> > - spin_lock(_window_list_lock);
> > - list_add(>list, _window_list);
> > - spin_unlock(_window_list_lock);
>
> I'd leave these 3 lines here and in find_existing_ddw_windows() (which
> would make ddw_list_add -> ddw_prop_alloc). In general you want to have
> less stuff to do on the failure path. kmalloc may fail and needs kfree
> but you can safely delay list_add (which cannot fail) and avoid having
> the lock help twice in the same function (one of them is hidden inside
> ddw_list_add).
> Not sure if this change is really needed after all. Thanks,
I understand this leads to better performance in case anything fails.
Also, I think list_add happening in the end is less error-prone (in
case the list is checked between list_add and a fail).
But what if we put it at the end?
What is the chance of a kzalloc of 4 pointers (struct direct_window)
failing after walk_system_ram_range?
Is it not worthy doing that for making enable_ddw() easier to
understand?
Best regards,
Leonardo
Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
On 18/08/2020 09:40, Leonardo Bras wrote:
> There are two functions adding DDW to the direct_window_list in a
> similar way, so create a ddw_list_add() to avoid duplicity and
> simplify those functions.
>
> Also, on enable_ddw(), add list_del() on out_free_window to allow
> removing the window from list if any error occurs.
>
> Signed-off-by: Leonardo Bras
> ---
> arch/powerpc/platforms/pseries/iommu.c | 42 --
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c
> b/arch/powerpc/platforms/pseries/iommu.c
> index 39617ce0ec83..fcdefcc0f365 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -872,6 +872,24 @@ static u64 find_existing_ddw(struct device_node *pdn)
> return dma_addr;
> }
>
> +static struct direct_window *ddw_list_add(struct device_node *pdn,
> + const struct dynamic_dma_window_prop
> *dma64)
> +{
> + struct direct_window *window;
> +
> + window = kzalloc(sizeof(*window), GFP_KERNEL);
> + if (!window)
> + return NULL;
> +
> + window->device = pdn;
> + window->prop = dma64;
> + spin_lock(_window_list_lock);
> + list_add(>list, _window_list);
> + spin_unlock(_window_list_lock);
> +
> + return window;
> +}
> +
> static int find_existing_ddw_windows(void)
> {
> int len;
> @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
> if (!direct64)
> continue;
>
> - window = kzalloc(sizeof(*window), GFP_KERNEL);
> - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
> + window = ddw_list_add(pdn, direct64);
> + if (!window || len < sizeof(*direct64)) {
Since you are touching this code, it looks like the "len <
sizeof(*direct64)" part should go above to "if (!direct64)".
> kfree(window);
> remove_ddw(pdn, true);
> - continue;
> }
> -
> - window->device = pdn;
> - window->prop = direct64;
> - spin_lock(_window_list_lock);
> - list_add(>list, _window_list);
> - spin_unlock(_window_list_lock);
> }
>
> return 0;
> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> device_node *pdn)
> dev_dbg(>dev, "created tce table LIOBN 0x%x for %pOF\n",
> create.liobn, dn);
>
> - window = kzalloc(sizeof(*window), GFP_KERNEL);
> + /* Add new window to existing DDW list */
The comment seems to duplicate what the ddw_list_add name already suggests.
> + window = ddw_list_add(pdn, ddwprop);
> if (!window)
> goto out_clear_window;
>
> @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct
> device_node *pdn)
> goto out_free_window;
> }
>
> - window->device = pdn;
> - window->prop = ddwprop;
> - spin_lock(_window_list_lock);
> - list_add(>list, _window_list);
> - spin_unlock(_window_list_lock);
I'd leave these 3 lines here and in find_existing_ddw_windows() (which
would make ddw_list_add -> ddw_prop_alloc). In general you want to have
less stuff to do on the failure path. kmalloc may fail and needs kfree
but you can safely delay list_add (which cannot fail) and avoid having
the lock help twice in the same function (one of them is hidden inside
ddw_list_add).
Not sure if this change is really needed after all. Thanks,
> -
> dma_addr = be64_to_cpu(ddwprop->dma_base);
> goto out_unlock;
>
> out_free_window:
> + spin_lock(_window_list_lock);
> + list_del(>list);
> + spin_unlock(_window_list_lock);
> +
> kfree(window);
>
> out_clear_window:
>
--
Alexey
[PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
There are two functions adding DDW to the direct_window_list in a
similar way, so create a ddw_list_add() to avoid duplicity and
simplify those functions.
Also, on enable_ddw(), add list_del() on out_free_window to allow
removing the window from list if any error occurs.
Signed-off-by: Leonardo Bras
---
arch/powerpc/platforms/pseries/iommu.c | 42 --
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/iommu.c
b/arch/powerpc/platforms/pseries/iommu.c
index 39617ce0ec83..fcdefcc0f365 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -872,6 +872,24 @@ static u64 find_existing_ddw(struct device_node *pdn)
return dma_addr;
}
+static struct direct_window *ddw_list_add(struct device_node *pdn,
+ const struct dynamic_dma_window_prop
*dma64)
+{
+ struct direct_window *window;
+
+ window = kzalloc(sizeof(*window), GFP_KERNEL);
+ if (!window)
+ return NULL;
+
+ window->device = pdn;
+ window->prop = dma64;
+ spin_lock(_window_list_lock);
+ list_add(>list, _window_list);
+ spin_unlock(_window_list_lock);
+
+ return window;
+}
+
static int find_existing_ddw_windows(void)
{
int len;
@@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
if (!direct64)
continue;
- window = kzalloc(sizeof(*window), GFP_KERNEL);
- if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
+ window = ddw_list_add(pdn, direct64);
+ if (!window || len < sizeof(*direct64)) {
kfree(window);
remove_ddw(pdn, true);
- continue;
}
-
- window->device = pdn;
- window->prop = direct64;
- spin_lock(_window_list_lock);
- list_add(>list, _window_list);
- spin_unlock(_window_list_lock);
}
return 0;
@@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
dev_dbg(>dev, "created tce table LIOBN 0x%x for %pOF\n",
create.liobn, dn);
- window = kzalloc(sizeof(*window), GFP_KERNEL);
+ /* Add new window to existing DDW list */
+ window = ddw_list_add(pdn, ddwprop);
if (!window)
goto out_clear_window;
@@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct
device_node *pdn)
goto out_free_window;
}
- window->device = pdn;
- window->prop = ddwprop;
- spin_lock(_window_list_lock);
- list_add(>list, _window_list);
- spin_unlock(_window_list_lock);
-
dma_addr = be64_to_cpu(ddwprop->dma_base);
goto out_unlock;
out_free_window:
+ spin_lock(_window_list_lock);
+ list_del(>list);
+ spin_unlock(_window_list_lock);
+
kfree(window);
out_clear_window:
--
2.25.4
