All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: John Garry <john.garry@huawei.com>
Cc: will@kernel.org, linux-kernel@vger.kernel.org,
	linuxarm@huawei.com, iommu@lists.linux-foundation.org,
	robin.murphy@arm.com
Subject: Re: [PATCH v4 2/3] iommu/iova: Avoid double-negatives in magazine helpers
Date: Mon, 18 Jan 2021 11:08:12 +0100	[thread overview]
Message-ID: <YAVeDOiKBEKZ2Tdq@myrica> (raw)
In-Reply-To: <69c30e85-4a72-a0e1-1e56-4ffbd0df5aba@huawei.com>

On Mon, Jan 18, 2021 at 09:24:17AM +0000, John Garry wrote:
> On 15/01/2021 17:30, Jean-Philippe Brucker wrote:
> > On Thu, Dec 10, 2020 at 02:23:08AM +0800, John Garry wrote:
> > > A similar crash to the following could be observed if initial CPU rcache
> > > magazine allocations fail in init_iova_rcaches():
> > 
> 
> thanks for having a look
> 
> > Any idea why that's happening?  This fix seems ok but if we're expecting
> > allocation failures for the loaded magazine then we could easily get it
> > for cpu_rcaches too, and get a similar abort at runtime.
> 
> It's not specifically that we expect them (allocation failures for the
> loaded magazine), rather we should make safe against it.
> 
> So could you be more specific in your concern for the cpu_rcache failure?
> cpu_rcache magazine assignment comes from this logic.

If this fails:

drivers/iommu/iova.c:847: rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());

then we'll get an Oops in __iova_rcache_get(). So if we're making the
module safer against magazine allocation failure, shouldn't we also
protect against cpu_rcaches allocation failure?

Thanks,
Jean

> 
> Anyway, logic like "if not full" or "if not empty" is poor as the outcome
> for NULL is ambiguous (maybe there's a better word) and the code is not safe
> against it, and so I replace with "if space" or "if have an IOVA",
> respectively.
> 
> Thanks,
> John
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

WARNING: multiple messages have this Message-ID (diff)
From: Jean-Philippe Brucker <jean-philippe@linaro.org>
To: John Garry <john.garry@huawei.com>
Cc: joro@8bytes.org, will@kernel.org, linux-kernel@vger.kernel.org,
	linuxarm@huawei.com, iommu@lists.linux-foundation.org,
	robin.murphy@arm.com
Subject: Re: [PATCH v4 2/3] iommu/iova: Avoid double-negatives in magazine helpers
Date: Mon, 18 Jan 2021 11:08:12 +0100	[thread overview]
Message-ID: <YAVeDOiKBEKZ2Tdq@myrica> (raw)
In-Reply-To: <69c30e85-4a72-a0e1-1e56-4ffbd0df5aba@huawei.com>

On Mon, Jan 18, 2021 at 09:24:17AM +0000, John Garry wrote:
> On 15/01/2021 17:30, Jean-Philippe Brucker wrote:
> > On Thu, Dec 10, 2020 at 02:23:08AM +0800, John Garry wrote:
> > > A similar crash to the following could be observed if initial CPU rcache
> > > magazine allocations fail in init_iova_rcaches():
> > 
> 
> thanks for having a look
> 
> > Any idea why that's happening?  This fix seems ok but if we're expecting
> > allocation failures for the loaded magazine then we could easily get it
> > for cpu_rcaches too, and get a similar abort at runtime.
> 
> It's not specifically that we expect them (allocation failures for the
> loaded magazine), rather we should make safe against it.
> 
> So could you be more specific in your concern for the cpu_rcache failure?
> cpu_rcache magazine assignment comes from this logic.

If this fails:

drivers/iommu/iova.c:847: rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size());

then we'll get an Oops in __iova_rcache_get(). So if we're making the
module safer against magazine allocation failure, shouldn't we also
protect against cpu_rcaches allocation failure?

Thanks,
Jean

> 
> Anyway, logic like "if not full" or "if not empty" is poor as the outcome
> for NULL is ambiguous (maybe there's a better word) and the code is not safe
> against it, and so I replace with "if space" or "if have an IOVA",
> respectively.
> 
> Thanks,
> John
> 

  reply	other threads:[~2021-01-18 10:08 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-09 18:23 [PATCH v4 0/3] iommu/iova: Solve longterm IOVA issue John Garry
2020-12-09 18:23 ` John Garry
2020-12-09 18:23 ` [PATCH v4 1/3] iommu/iova: Add free_all_cpu_cached_iovas() John Garry
2020-12-09 18:23   ` John Garry
2021-01-15 17:28   ` Jean-Philippe Brucker
2021-01-15 17:28     ` Jean-Philippe Brucker
2020-12-09 18:23 ` [PATCH v4 2/3] iommu/iova: Avoid double-negatives in magazine helpers John Garry
2020-12-09 18:23   ` John Garry
2021-01-15 17:30   ` Jean-Philippe Brucker
2021-01-15 17:30     ` Jean-Philippe Brucker
2021-01-18  9:24     ` John Garry
2021-01-18  9:24       ` John Garry
2021-01-18 10:08       ` Jean-Philippe Brucker [this message]
2021-01-18 10:08         ` Jean-Philippe Brucker
2021-01-18 10:55         ` John Garry
2021-01-18 10:55           ` John Garry
2021-01-18 12:40           ` Jean-Philippe Brucker
2021-01-18 12:40             ` Jean-Philippe Brucker
2021-01-18 12:59           ` Robin Murphy
2021-01-18 12:59             ` Robin Murphy
2021-01-18 15:09             ` John Garry
2021-01-18 15:09               ` John Garry
2020-12-09 18:23 ` [PATCH v4 3/3] iommu/iova: Flush CPU rcache for when a depot fills John Garry
2020-12-09 18:23   ` John Garry
2021-01-15 17:32   ` Jean-Philippe Brucker
2021-01-15 17:32     ` Jean-Philippe Brucker
2021-01-15 19:21     ` Robin Murphy
2021-01-15 19:21       ` Robin Murphy
2021-01-18 12:38       ` John Garry
2021-01-18 12:38         ` John Garry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YAVeDOiKBEKZ2Tdq@myrica \
    --to=jean-philippe@linaro.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=john.garry@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=robin.murphy@arm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.