[1/6] swiotlb: Add helper to know if it is in use for a specific device.

Commit Message

Jerome Glisse Aug. 26, 2015, 6:52 p.m. UTC

From: Jérôme Glisse <[e-mail]>

Some device like GPU do things differently if swiotlb is in use. We
use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
but this is unreliable. Patch add a simple helpers to check if any of
the dma_ops associated with a device points to the swiotlb functions,
making swiotlb check reliable for a device.

Signed-off-by: Jérôme Glisse <[e-mail]>
Cc: Konrad Rzeszutek Wilk <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Ben Skeggs <[e-mail]>
Cc: Dave Airlie <[e-mail]>
Cc: [e-mail]
Cc: Daniel Vetter <[e-mail]>
---
 include/linux/dma-mapping.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Konrad Rzeszutek Wilk Aug. 26, 2015, 7:02 p.m. UTC | #1

On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> From: Jérôme Glisse <[e-mail]>
> 
> Some device like GPU do things differently if swiotlb is in use. We
> use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> but this is unreliable. Patch add a simple helpers to check if any of

Why is it unreliable?

> the dma_ops associated with a device points to the swiotlb functions,
> making swiotlb check reliable for a device.
> 
> Signed-off-by: Jérôme Glisse <[e-mail]>
> Cc: Konrad Rzeszutek Wilk <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Ben Skeggs <[e-mail]>
> Cc: Dave Airlie <[e-mail]>
> Cc: [e-mail]
> Cc: Daniel Vetter <[e-mail]>
> ---
>  include/linux/dma-mapping.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index ac07ff0..eac911e 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
>  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
>  #endif
>  
> +
> +#ifdef CONFIG_SWIOTLB
> +static inline bool swiotlb_in_use(struct device *dev)
> +{
> +	struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> +		ops->map_page == swiotlb_map_page);

That won't work. What if we use xen-swiotlb which has different function
names?

> +}
> +#else
> +static inline bool swiotlb_in_use(struct device *dev)
> +{
> +	return false;
> +}
> +#endif
> +
> +
>  #endif
> -- 
> 2.1.0
>

Jerome Glisse Aug. 26, 2015, 7:26 p.m. UTC | #2

On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> > From: Jérôme Glisse <[e-mail]>
> > 
> > Some device like GPU do things differently if swiotlb is in use. We
> > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > but this is unreliable. Patch add a simple helpers to check if any of
> 
> Why is it unreliable?

Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
which cleanup swiotlb init and always allocate default size. Which i believe
is a waste of memory. So we need to add a real helper to know if swiotlb is
in use or not and we should not rely on expectation of some swiotlb value.

> 
> > the dma_ops associated with a device points to the swiotlb functions,
> > making swiotlb check reliable for a device.
> > 
> > Signed-off-by: Jérôme Glisse <[e-mail]>
> > Cc: Konrad Rzeszutek Wilk <[e-mail]>
> > Cc: Alex Deucher <[e-mail]>
> > Cc: Ben Skeggs <[e-mail]>
> > Cc: Dave Airlie <[e-mail]>
> > Cc: [e-mail]
> > Cc: Daniel Vetter <[e-mail]>
> > ---
> >  include/linux/dma-mapping.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index ac07ff0..eac911e 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
> >  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
> >  #endif
> >  
> > +
> > +#ifdef CONFIG_SWIOTLB
> > +static inline bool swiotlb_in_use(struct device *dev)
> > +{
> > +	struct dma_map_ops *ops = get_dma_ops(dev);
> > +
> > +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> > +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > +		ops->map_page == swiotlb_map_page);
> 
> That won't work. What if we use xen-swiotlb which has different function
> names?

I didn't thought about xen, always doing things differently, i think xen is
just a matter of also testing for the xen function. I just wanted to have
the helper in common code and only rely on common things, instead of having
to add a per arch helper.

Cheers,
Jérôme

Konrad Rzeszutek Wilk Aug. 26, 2015, 7:44 p.m. UTC | #3

On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> > > From: Jérôme Glisse <[e-mail]>
> > > 
> > > Some device like GPU do things differently if swiotlb is in use. We
> > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > but this is unreliable. Patch add a simple helpers to check if any of
> > 
> > Why is it unreliable?
> 
> Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
> which cleanup swiotlb init and always allocate default size. Which i believe
> is a waste of memory. So we need to add a real helper to know if swiotlb is
> in use or not and we should not rely on expectation of some swiotlb value.

Ah right, that patch. That should have been part of the description
I believe.

> 
> > 
> > > the dma_ops associated with a device points to the swiotlb functions,
> > > making swiotlb check reliable for a device.
> > > 
> > > Signed-off-by: Jérôme Glisse <[e-mail]>
> > > Cc: Konrad Rzeszutek Wilk <[e-mail]>
> > > Cc: Alex Deucher <[e-mail]>
> > > Cc: Ben Skeggs <[e-mail]>
> > > Cc: Dave Airlie <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Daniel Vetter <[e-mail]>
> > > ---
> > >  include/linux/dma-mapping.h | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > index ac07ff0..eac911e 100644
> > > --- a/include/linux/dma-mapping.h
> > > +++ b/include/linux/dma-mapping.h
> > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
> > >  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
> > >  #endif
> > >  
> > > +
> > > +#ifdef CONFIG_SWIOTLB
> > > +static inline bool swiotlb_in_use(struct device *dev)
> > > +{
> > > +	struct dma_map_ops *ops = get_dma_ops(dev);
> > > +
> > > +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > +		ops->map_page == swiotlb_map_page);
> > 
> > That won't work. What if we use xen-swiotlb which has different function
> > names?
> 
> I didn't thought about xen, always doing things differently, i think xen is
> just a matter of also testing for the xen function. I just wanted to have
> the helper in common code and only rely on common things, instead of having
> to add a per arch helper.

There has to be a better way. Perhaps you can expand SWIOTLB to actually
check if it is in use?
> 
> Cheers,
> Jérôme

Jerome Glisse Aug. 26, 2015, 8:31 p.m. UTC | #4

On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> > > > From: Jérôme Glisse <[e-mail]>
> > > > 
> > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > > but this is unreliable. Patch add a simple helpers to check if any of
> > > 
> > > Why is it unreliable?
> > 
> > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> > is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
> > which cleanup swiotlb init and always allocate default size. Which i believe
> > is a waste of memory. So we need to add a real helper to know if swiotlb is
> > in use or not and we should not rely on expectation of some swiotlb value.
> 
> Ah right, that patch. That should have been part of the description
> I believe.
> 
> > 
> > > 
> > > > the dma_ops associated with a device points to the swiotlb functions,
> > > > making swiotlb check reliable for a device.
> > > > 
> > > > Signed-off-by: Jérôme Glisse <[e-mail]>
> > > > Cc: Konrad Rzeszutek Wilk <[e-mail]>
> > > > Cc: Alex Deucher <[e-mail]>
> > > > Cc: Ben Skeggs <[e-mail]>
> > > > Cc: Dave Airlie <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: Daniel Vetter <[e-mail]>
> > > > ---
> > > >  include/linux/dma-mapping.h | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index ac07ff0..eac911e 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
> > > >  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
> > > >  #endif
> > > >  
> > > > +
> > > > +#ifdef CONFIG_SWIOTLB
> > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > +{
> > > > +	struct dma_map_ops *ops = get_dma_ops(dev);
> > > > +
> > > > +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > +		ops->map_page == swiotlb_map_page);
> > > 
> > > That won't work. What if we use xen-swiotlb which has different function
> > > names?
> > 
> > I didn't thought about xen, always doing things differently, i think xen is
> > just a matter of also testing for the xen function. I just wanted to have
> > the helper in common code and only rely on common things, instead of having
> > to add a per arch helper.
> 
> There has to be a better way. Perhaps you can expand SWIOTLB to actually
> check if it is in use?

This would require per arch modifications which is what i was trying to avoid.

Cheers,
Jérôme

Konrad Rzeszutek Wilk Aug. 26, 2015, 8:38 p.m. UTC | #5

On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
> On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> > > > > From: Jérôme Glisse <[e-mail]>
> > > > > 
> > > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > > > but this is unreliable. Patch add a simple helpers to check if any of
> > > > 
> > > > Why is it unreliable?
> > > 
> > > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> > > is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
> > > which cleanup swiotlb init and always allocate default size. Which i believe
> > > is a waste of memory. So we need to add a real helper to know if swiotlb is
> > > in use or not and we should not rely on expectation of some swiotlb value.
> > 
> > Ah right, that patch. That should have been part of the description
> > I believe.
> > 
> > > 
> > > > 
> > > > > the dma_ops associated with a device points to the swiotlb functions,
> > > > > making swiotlb check reliable for a device.
> > > > > 
> > > > > Signed-off-by: Jérôme Glisse <[e-mail]>
> > > > > Cc: Konrad Rzeszutek Wilk <[e-mail]>
> > > > > Cc: Alex Deucher <[e-mail]>
> > > > > Cc: Ben Skeggs <[e-mail]>
> > > > > Cc: Dave Airlie <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Cc: Daniel Vetter <[e-mail]>
> > > > > ---
> > > > >  include/linux/dma-mapping.h | 18 ++++++++++++++++++
> > > > >  1 file changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > > index ac07ff0..eac911e 100644
> > > > > --- a/include/linux/dma-mapping.h
> > > > > +++ b/include/linux/dma-mapping.h
> > > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
> > > > >  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
> > > > >  #endif
> > > > >  
> > > > > +
> > > > > +#ifdef CONFIG_SWIOTLB
> > > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > > +{
> > > > > +	struct dma_map_ops *ops = get_dma_ops(dev);
> > > > > +
> > > > > +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > > +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > > +		ops->map_page == swiotlb_map_page);
> > > > 
> > > > That won't work. What if we use xen-swiotlb which has different function
> > > > names?
> > > 
> > > I didn't thought about xen, always doing things differently, i think xen is
> > > just a matter of also testing for the xen function. I just wanted to have
> > > the helper in common code and only rely on common things, instead of having
> > > to add a per arch helper.
> > 
> > There has to be a better way. Perhaps you can expand SWIOTLB to actually
> > check if it is in use?
> 
> This would require per arch modifications which is what i was trying to avoid.

How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used
then the modifications are only in the lib/swiotlb.c ?
> 
> Cheers,
> Jérôme

Jerome Glisse Aug. 26, 2015, 9:10 p.m. UTC | #6

On Wed, Aug 26, 2015 at 04:38:14PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
> > On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
> > > > On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > On Wed, Aug 26, 2015 at 02:52:02PM -0400, [e-mail] wrote:
> > > > > > From: Jérôme Glisse <[e-mail]>
> > > > > > 
> > > > > > Some device like GPU do things differently if swiotlb is in use. We
> > > > > > use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not
> > > > > > but this is unreliable. Patch add a simple helpers to check if any of
> > > > > 
> > > > > Why is it unreliable?
> > > > 
> > > > Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb
> > > > is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f
> > > > which cleanup swiotlb init and always allocate default size. Which i believe
> > > > is a waste of memory. So we need to add a real helper to know if swiotlb is
> > > > in use or not and we should not rely on expectation of some swiotlb value.
> > > 
> > > Ah right, that patch. That should have been part of the description
> > > I believe.
> > > 
> > > > 
> > > > > 
> > > > > > the dma_ops associated with a device points to the swiotlb functions,
> > > > > > making swiotlb check reliable for a device.
> > > > > > 
> > > > > > Signed-off-by: Jérôme Glisse <[e-mail]>
> > > > > > Cc: Konrad Rzeszutek Wilk <[e-mail]>
> > > > > > Cc: Alex Deucher <[e-mail]>
> > > > > > Cc: Ben Skeggs <[e-mail]>
> > > > > > Cc: Dave Airlie <[e-mail]>
> > > > > > Cc: [e-mail]
> > > > > > Cc: Daniel Vetter <[e-mail]>
> > > > > > ---
> > > > > >  include/linux/dma-mapping.h | 18 ++++++++++++++++++
> > > > > >  1 file changed, 18 insertions(+)
> > > > > > 
> > > > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > > > index ac07ff0..eac911e 100644
> > > > > > --- a/include/linux/dma-mapping.h
> > > > > > +++ b/include/linux/dma-mapping.h
> > > > > > @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev,
> > > > > >  #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
> > > > > >  #endif
> > > > > >  
> > > > > > +
> > > > > > +#ifdef CONFIG_SWIOTLB
> > > > > > +static inline bool swiotlb_in_use(struct device *dev)
> > > > > > +{
> > > > > > +	struct dma_map_ops *ops = get_dma_ops(dev);
> > > > > > +
> > > > > > +	return (ops->map_sg == swiotlb_map_sg_attrs ||
> > > > > > +		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
> > > > > > +		ops->map_page == swiotlb_map_page);
> > > > > 
> > > > > That won't work. What if we use xen-swiotlb which has different function
> > > > > names?
> > > > 
> > > > I didn't thought about xen, always doing things differently, i think xen is
> > > > just a matter of also testing for the xen function. I just wanted to have
> > > > the helper in common code and only rely on common things, instead of having
> > > > to add a per arch helper.
> > > 
> > > There has to be a better way. Perhaps you can expand SWIOTLB to actually
> > > check if it is in use?
> > 
> > This would require per arch modifications which is what i was trying to avoid.
> 
> How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used
> then the modifications are only in the lib/swiotlb.c ?

I am not sure i follow swiotlb_nr_tbl is an internal value that so far have only
be use by driver through calling swiotlb_nr_tbl() so calls to that functions does
not reflect if swiotlb is enabled or not.

In fact i am pretty sure only arch specific code stores information on wether or
not swiotlb is enabled. Beside i think some device overide dev->archdata.dma_ops
which effectively disable swiotlb for the device.

But if you really dislike just testing dma_ops against swiotlb & xen_swiotlb i
will respin with arch mod but i will only handle ppc/arm/x86.

Cheers,
Jérôme

Patch

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index ac07ff0..eac911e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -314,4 +314,22 @@  static inline int dma_mmap_writecombine(struct device *dev,
 #define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
 #endif
 
+
+#ifdef CONFIG_SWIOTLB
+static inline bool swiotlb_in_use(struct device *dev)
+{
+	struct dma_map_ops *ops = get_dma_ops(dev);
+
+	return (ops->map_sg == swiotlb_map_sg_attrs ||
+		ops->unmap_sg == swiotlb_unmap_sg_attrs ||
+		ops->map_page == swiotlb_map_page);
+}
+#else
+static inline bool swiotlb_in_use(struct device *dev)
+{
+	return false;
+}
+#endif
+
+
 #endif