| Message ID | 20230212204454.2938561-6-ogabbay@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/27] habanalabs/gaudi2: increase user interrupt grace time | expand |
On Sun, Feb 12, 2023 at 10:44:33PM +0200, Oded Gabbay wrote: > From: Tomer Tayar <ttayar@habana.ai> > > The same mutex lock/unlock and counter decrementing in > hl_release_dmabuf() is already done in the memhash_node_export_put() > helper function. > > Signed-off-by: Tomer Tayar <ttayar@habana.ai> > Reviewed-by: Oded Gabbay <ogabbay@kernel.org> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org> > --- > drivers/accel/habanalabs/common/memory.c | 89 ++++++++++++------------ > 1 file changed, 43 insertions(+), 46 deletions(-) > > diff --git a/drivers/accel/habanalabs/common/memory.c b/drivers/accel/habanalabs/common/memory.c > index e6474d38afc4..7b3809853dd5 100644 > --- a/drivers/accel/habanalabs/common/memory.c > +++ b/drivers/accel/habanalabs/common/memory.c > @@ -1779,6 +1779,47 @@ static void hl_unmap_dmabuf(struct dma_buf_attachment *attachment, > kfree(sgt); > } > > +static struct hl_vm_hash_node *memhash_node_export_get(struct hl_ctx *ctx, u64 addr) > +{ > + struct hl_device *hdev = ctx->hdev; > + struct hl_vm_hash_node *hnode; > + > + /* get the memory handle */ > + mutex_lock(&ctx->mem_hash_lock); > + hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr) > + if (addr == hnode->vaddr) > + break; > + > + if (!hnode) { This looks suspicious, I think hnode can be not-NULL here and has hnode->vaddr different than searched addr, in case there is hash collision and we iterate over hlist where there is no searched addr. Not 100% sure about that though. I think would be better to provide helper like this: hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr) if (addr == hnode->vaddr) return hnode; return NULL; which is basically standard way how hash_for_each_possible() used. Regards Stanislaw
On Thu, Feb 16, 2023 at 13:48 Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > The same mutex lock/unlock and counter decrementing in > > hl_release_dmabuf() is already done in the memhash_node_export_put() > > helper function. > > > > Signed-off-by: Tomer Tayar <ttayar@habana.ai> > > Reviewed-by: Oded Gabbay <ogabbay@kernel.org> > > Signed-off-by: Oded Gabbay <ogabbay@kernel.org> > > --- > > drivers/accel/habanalabs/common/memory.c | 89 ++++++++++++---------- > -- > > 1 file changed, 43 insertions(+), 46 deletions(-) > > > > diff --git a/drivers/accel/habanalabs/common/memory.c > b/drivers/accel/habanalabs/common/memory.c > > index e6474d38afc4..7b3809853dd5 100644 > > --- a/drivers/accel/habanalabs/common/memory.c > > +++ b/drivers/accel/habanalabs/common/memory.c > > @@ -1779,6 +1779,47 @@ static void hl_unmap_dmabuf(struct > dma_buf_attachment *attachment, > > kfree(sgt); > > } > > > > +static struct hl_vm_hash_node *memhash_node_export_get(struct hl_ctx > *ctx, u64 addr) > > +{ > > + struct hl_device *hdev = ctx->hdev; > > + struct hl_vm_hash_node *hnode; > > + > > + /* get the memory handle */ > > + mutex_lock(&ctx->mem_hash_lock); > > + hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned > long)addr) > > + if (addr == hnode->vaddr) > > + break; > > + > > + if (!hnode) { > > This looks suspicious, I think hnode can be not-NULL here and has > hnode->vaddr different than searched addr, in case there is > hash collision and we iterate over hlist where there is no > searched addr. Not 100% sure about that though. > > I think would be better to provide helper like this: > > hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned > long)addr) > if (addr == hnode->vaddr) > return hnode; > return NULL; > > which is basically standard way how hash_for_each_possible() used. > > > Regards > Stanislaw Thanks Stanislaw, we will add such a helper and use it here and in another place with a similar pattern. If that is okay, we will do it in another patch, as this one only moves an existing function for code reuse.
On Thu, Feb 16, 2023 at 02:26:50PM +0000, Tomer Tayar wrote: > > This looks suspicious, I think hnode can be not-NULL here and has > > hnode->vaddr different than searched addr, in case there is > > hash collision and we iterate over hlist where there is no > > searched addr. Not 100% sure about that though. > > > > I think would be better to provide helper like this: > > > > hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned > > long)addr) > > if (addr == hnode->vaddr) > > return hnode; > > return NULL; > > > > which is basically standard way how hash_for_each_possible() used. > > > > > > Regards > > Stanislaw > > Thanks Stanislaw, we will add such a helper and use it here and in another place with a similar pattern. > If that is okay, we will do it in another patch, as this one only moves an existing function for code reuse. Sure, no problem with that. Regards Stanislaw
diff --git a/drivers/accel/habanalabs/common/memory.c b/drivers/accel/habanalabs/common/memory.c index e6474d38afc4..7b3809853dd5 100644 --- a/drivers/accel/habanalabs/common/memory.c +++ b/drivers/accel/habanalabs/common/memory.c @@ -1779,6 +1779,47 @@ static void hl_unmap_dmabuf(struct dma_buf_attachment *attachment, kfree(sgt); } +static struct hl_vm_hash_node *memhash_node_export_get(struct hl_ctx *ctx, u64 addr) +{ + struct hl_device *hdev = ctx->hdev; + struct hl_vm_hash_node *hnode; + + /* get the memory handle */ + mutex_lock(&ctx->mem_hash_lock); + hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr) + if (addr == hnode->vaddr) + break; + + if (!hnode) { + mutex_unlock(&ctx->mem_hash_lock); + dev_dbg(hdev->dev, "map address %#llx not found\n", addr); + return ERR_PTR(-EINVAL); + } + + if (upper_32_bits(hnode->handle)) { + mutex_unlock(&ctx->mem_hash_lock); + dev_dbg(hdev->dev, "invalid handle %#llx for map address %#llx\n", + hnode->handle, addr); + return ERR_PTR(-EINVAL); + } + + /* + * node found, increase export count so this memory cannot be unmapped + * and the hash node cannot be deleted. + */ + hnode->export_cnt++; + mutex_unlock(&ctx->mem_hash_lock); + + return hnode; +} + +static void memhash_node_export_put(struct hl_ctx *ctx, struct hl_vm_hash_node *hnode) +{ + mutex_lock(&ctx->mem_hash_lock); + hnode->export_cnt--; + mutex_unlock(&ctx->mem_hash_lock); +} + static void hl_release_dmabuf(struct dma_buf *dmabuf) { struct hl_dmabuf_priv *hl_dmabuf = dmabuf->priv; @@ -1789,11 +1830,8 @@ static void hl_release_dmabuf(struct dma_buf *dmabuf) ctx = hl_dmabuf->ctx; - if (hl_dmabuf->memhash_hnode) { - mutex_lock(&ctx->mem_hash_lock); - hl_dmabuf->memhash_hnode->export_cnt--; - mutex_unlock(&ctx->mem_hash_lock); - } + if (hl_dmabuf->memhash_hnode) + memhash_node_export_put(ctx, hl_dmabuf->memhash_hnode); hl_ctx_put(ctx); kfree(hl_dmabuf); @@ -1933,47 +1971,6 @@ static int validate_export_params(struct hl_device *hdev, u64 device_addr, u64 s return 0; } -static struct hl_vm_hash_node *memhash_node_export_get(struct hl_ctx *ctx, u64 addr) -{ - struct hl_device *hdev = ctx->hdev; - struct hl_vm_hash_node *hnode; - - /* get the memory handle */ - mutex_lock(&ctx->mem_hash_lock); - hash_for_each_possible(ctx->mem_hash, hnode, node, (unsigned long)addr) - if (addr == hnode->vaddr) - break; - - if (!hnode) { - mutex_unlock(&ctx->mem_hash_lock); - dev_dbg(hdev->dev, "map address %#llx not found\n", addr); - return ERR_PTR(-EINVAL); - } - - if (upper_32_bits(hnode->handle)) { - mutex_unlock(&ctx->mem_hash_lock); - dev_dbg(hdev->dev, "invalid handle %#llx for map address %#llx\n", - hnode->handle, addr); - return ERR_PTR(-EINVAL); - } - - /* - * node found, increase export count so this memory cannot be unmapped - * and the hash node cannot be deleted. - */ - hnode->export_cnt++; - mutex_unlock(&ctx->mem_hash_lock); - - return hnode; -} - -static void memhash_node_export_put(struct hl_ctx *ctx, struct hl_vm_hash_node *hnode) -{ - mutex_lock(&ctx->mem_hash_lock); - hnode->export_cnt--; - mutex_unlock(&ctx->mem_hash_lock); -} - static struct hl_vm_phys_pg_pack *get_phys_pg_pack_from_hash_node(struct hl_device *hdev, struct hl_vm_hash_node *hnode) {