Message ID | 20200817234033.442511-7-leobras.c@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | DDW indirect mapping | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (97a94d178e5876ad49482c42b13b7296cd6803de) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (388692e943a58f28aac0fe83e75f5994da9ff8a1) |
snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (0f1fa5848ab32d269a2030caac618bd6a99ab3f3) |
snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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 <leobras.c@gmail.com> > --- > 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(&direct_window_list_lock); > + list_add(&window->list, &direct_window_list); > + spin_unlock(&direct_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(&direct_window_list_lock); > - list_add(&window->list, &direct_window_list); > - spin_unlock(&direct_window_list_lock); > } > > return 0; > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > dev_dbg(&dev->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(&direct_window_list_lock); > - list_add(&window->list, &direct_window_list); > - spin_unlock(&direct_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(&direct_window_list_lock); > + list_del(&window->list); > + spin_unlock(&direct_window_list_lock); > + > kfree(window); > > out_clear_window: >
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(&direct_window_list_lock); > > - list_add(&window->list, &direct_window_list); > > - spin_unlock(&direct_window_list_lock); > > } > > > > return 0; > > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > dev_dbg(&dev->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(&direct_window_list_lock); > > - list_add(&window->list, &direct_window_list); > > - spin_unlock(&direct_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
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(&direct_window_list_lock); >>> - list_add(&window->list, &direct_window_list); >>> - spin_unlock(&direct_window_list_lock); >>> } >>> >>> return 0; >>> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) >>> dev_dbg(&dev->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(&direct_window_list_lock); >>> - list_add(&window->list, &direct_window_list); >>> - spin_unlock(&direct_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 >
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(&direct_window_list_lock); > > > > - list_add(&window->list, &direct_window_list); > > > > - spin_unlock(&direct_window_list_lock); > > > > } > > > > > > > > return 0; > > > > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > > > dev_dbg(&dev->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(&direct_window_list_lock); > > > > - list_add(&window->list, &direct_window_list); > > > > - spin_unlock(&direct_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
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(&direct_window_list_lock); + list_add(&window->list, &direct_window_list); + spin_unlock(&direct_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(&direct_window_list_lock); - list_add(&window->list, &direct_window_list); - spin_unlock(&direct_window_list_lock); } return 0; @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) dev_dbg(&dev->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(&direct_window_list_lock); - list_add(&window->list, &direct_window_list); - spin_unlock(&direct_window_list_lock); - dma_addr = be64_to_cpu(ddwprop->dma_base); goto out_unlock; out_free_window: + spin_lock(&direct_window_list_lock); + list_del(&window->list); + spin_unlock(&direct_window_list_lock); + kfree(window); out_clear_window:
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 <leobras.c@gmail.com> --- arch/powerpc/platforms/pseries/iommu.c | 42 ++++++++++++++++---------- 1 file changed, 26 insertions(+), 16 deletions(-)