[PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This patch solves a crash at the time of mlx4 driver unload or system
shutdown. The crash occurs because dma_alloc_coherent() returns one
value in mlx4_alloc_icm_coherent(), but a different value is passed to
dma_free_coherent() in mlx4_free_icm_coherent(). In turn this is because
when allocated, that pointer is passed to sg_set_buf() to record it,
then when freed it is re-calculated by calling
lowmem_page_address(sg_page()) which returns a different value. Solve
this by recording the value that dma_alloc_coherent() returns, and
passing this to dma_free_coherent().

This patch is roughly equivalent to commit 378efe798ecf ("RDMA/hns: Get
rid of page operation after dma_alloc_coherent"). That patch was
described as:

> In general, dma_alloc_coherent() returns a CPU virtual address and
> a DMA address, and we have no guarantee that the underlying memory
> even has an associated struct page at all.
>
> This patch gets rid of the page operation after dma_alloc_coherent,
> and records the VA returned form dma_alloc_coherent in the struct
> of hem in hns RoCE driver.

However, this patch reworks the code to store all information about
coherent chunks separately from the sg list, since using sg lists for
them doesn't make sense. Hence, the structure of this patch is quite
different compared to the hns patch.

Based-on-code-from: Christoph Hellwig <hch@lst.de>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3:
- Rework chunk data structure to store all data for coherent allocations
  separately from the sg list. Code from Christoph Hellwig with fixes by
  me. Notes:
  - chunk->coherent is an int not a bool since checkpatch complains about
    using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
  - chunk->coherent is used rather than chunk->table->coherent since the
    table pointer isn't available when creating chunks. This duplicates
    data, but simplifies the patch.
v2:
- Rework mlx4_table_find() to explicitly calculate the returned address
  differently depending on wheter the table was allocated using
  dma_alloc_coherent() or alloc_pages(), which in turn allows the
  changes to mlx4_alloc_icm_pages() to be dropped.
- Drop changes to mlx4_alloc/free_icm_pages. This path uses
  pci_map_sg() which can re-write the sg list which in turn would cause
  chunk->mem[] (the sg list) and chunk->buf[] to become inconsistent.
- Enhance commit description.

Note: I've tested this patch in a downstream 4.14 based kernel (using
ibping, ib_read_bw, and ib_write_bw), but can't test it in mainline
since my system isn't supported there yet. I have compile-tested it in
mainline at least, for ARM64.
---
 drivers/net/ethernet/mellanox/mlx4/icm.c | 92 ++++++++++++++----------
 drivers/net/ethernet/mellanox/mlx4/icm.h | 22 +++++-
 2 files changed, 75 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
index 4b4351141b94..76b84d08a058 100644
--- a/drivers/net/ethernet/mellanox/mlx4/icm.c
+++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
@@ -57,12 +57,12 @@ static void mlx4_free_icm_pages(struct mlx4_dev *dev, struct mlx4_icm_chunk *chu
 	int i;
 
 	if (chunk->nsg > 0)
-		pci_unmap_sg(dev->persist->pdev, chunk->mem, chunk->npages,
+		pci_unmap_sg(dev->persist->pdev, chunk->sg, chunk->npages,
 			     PCI_DMA_BIDIRECTIONAL);
 
 	for (i = 0; i < chunk->npages; ++i)
-		__free_pages(sg_page(&chunk->mem[i]),
-			     get_order(chunk->mem[i].length));
+		__free_pages(sg_page(&chunk->sg[i]),
+			     get_order(chunk->sg[i].length));
 }
 
 static void mlx4_free_icm_coherent(struct mlx4_dev *dev, struct mlx4_icm_chunk *chunk)
@@ -71,9 +71,9 @@ static void mlx4_free_icm_coherent(struct mlx4_dev *dev, struct mlx4_icm_chunk *
 
 	for (i = 0; i < chunk->npages; ++i)
 		dma_free_coherent(&dev->persist->pdev->dev,
-				  chunk->mem[i].length,
-				  lowmem_page_address(sg_page(&chunk->mem[i])),
-				  sg_dma_address(&chunk->mem[i]));
+				  chunk->buf[i].size,
+				  chunk->buf[i].addr,
+				  chunk->buf[i].dma_addr);
 }
 
 void mlx4_free_icm(struct mlx4_dev *dev, struct mlx4_icm *icm, int coherent)
@@ -111,22 +111,21 @@ static int mlx4_alloc_icm_pages(struct scatterlist *mem, int order,
 	return 0;
 }
 
-static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
-				    int order, gfp_t gfp_mask)
+static int mlx4_alloc_icm_coherent(struct device *dev, struct mlx4_icm_buf *buf,
+				   int order, gfp_t gfp_mask)
 {
-	void *buf = dma_alloc_coherent(dev, PAGE_SIZE << order,
-				       &sg_dma_address(mem), gfp_mask);
-	if (!buf)
+	buf->addr = dma_alloc_coherent(dev, PAGE_SIZE << order,
+				       &buf->dma_addr, gfp_mask);
+	if (!buf->addr)
 		return -ENOMEM;
 
-	if (offset_in_page(buf)) {
-		dma_free_coherent(dev, PAGE_SIZE << order,
-				  buf, sg_dma_address(mem));
+	if (offset_in_page(buf->addr)) {
+		dma_free_coherent(dev, PAGE_SIZE << order, buf->addr,
+				  buf->dma_addr);
 		return -ENOMEM;
 	}
 
-	sg_set_buf(mem, buf, PAGE_SIZE << order);
-	sg_dma_len(mem) = PAGE_SIZE << order;
+	buf->size = PAGE_SIZE << order;
 	return 0;
 }
 
@@ -159,21 +158,21 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
 
 	while (npages > 0) {
 		if (!chunk) {
-			chunk = kmalloc_node(sizeof(*chunk),
+			chunk = kzalloc_node(sizeof(*chunk),
 					     gfp_mask & ~(__GFP_HIGHMEM |
 							  __GFP_NOWARN),
 					     dev->numa_node);
 			if (!chunk) {
-				chunk = kmalloc(sizeof(*chunk),
+				chunk = kzalloc(sizeof(*chunk),
 						gfp_mask & ~(__GFP_HIGHMEM |
 							     __GFP_NOWARN));
 				if (!chunk)
 					goto fail;
 			}
+			chunk->coherent = coherent;
 
-			sg_init_table(chunk->mem, MLX4_ICM_CHUNK_LEN);
-			chunk->npages = 0;
-			chunk->nsg    = 0;
+			if (!coherent)
+				sg_init_table(chunk->sg, MLX4_ICM_CHUNK_LEN);
 			list_add_tail(&chunk->list, &icm->chunk_list);
 		}
 
@@ -186,10 +185,10 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
 
 		if (coherent)
 			ret = mlx4_alloc_icm_coherent(&dev->persist->pdev->dev,
-						      &chunk->mem[chunk->npages],
-						      cur_order, mask);
+						&chunk->buf[chunk->npages],
+						cur_order, mask);
 		else
-			ret = mlx4_alloc_icm_pages(&chunk->mem[chunk->npages],
+			ret = mlx4_alloc_icm_pages(&chunk->sg[chunk->npages],
 						   cur_order, mask,
 						   dev->numa_node);
 
@@ -205,7 +204,7 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
 		if (coherent)
 			++chunk->nsg;
 		else if (chunk->npages == MLX4_ICM_CHUNK_LEN) {
-			chunk->nsg = pci_map_sg(dev->persist->pdev, chunk->mem,
+			chunk->nsg = pci_map_sg(dev->persist->pdev, chunk->sg,
 						chunk->npages,
 						PCI_DMA_BIDIRECTIONAL);
 
@@ -220,7 +219,7 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
 	}
 
 	if (!coherent && chunk) {
-		chunk->nsg = pci_map_sg(dev->persist->pdev, chunk->mem,
+		chunk->nsg = pci_map_sg(dev->persist->pdev, chunk->sg,
 					chunk->npages,
 					PCI_DMA_BIDIRECTIONAL);
 
@@ -320,7 +319,7 @@ void *mlx4_table_find(struct mlx4_icm_table *table, u32 obj,
 	u64 idx;
 	struct mlx4_icm_chunk *chunk;
 	struct mlx4_icm *icm;
-	struct page *page = NULL;
+	void *addr = NULL;
 
 	if (!table->lowmem)
 		return NULL;
@@ -336,28 +335,49 @@ void *mlx4_table_find(struct mlx4_icm_table *table, u32 obj,
 
 	list_for_each_entry(chunk, &icm->chunk_list, list) {
 		for (i = 0; i < chunk->npages; ++i) {
+			dma_addr_t dma_addr;
+			size_t len;
+
+			if (table->coherent) {
+				len = chunk->buf[i].size;
+				dma_addr = chunk->buf[i].dma_addr;
+				addr = chunk->buf[i].addr;
+			} else {
+				struct page *page;
+
+				len = sg_dma_len(&chunk->sg[i]);
+				dma_addr = sg_dma_address(&chunk->sg[i]);
+
+				/* XXX: we should never do this for highmem
+				 * allocation.  This function either needs
+				 * to be split, or the kernel virtual address
+				 * return needs to be made optional.
+				 */
+				page = sg_page(&chunk->sg[i]);
+				addr = lowmem_page_address(page);
+			}
+
 			if (dma_handle && dma_offset >= 0) {
-				if (sg_dma_len(&chunk->mem[i]) > dma_offset)
-					*dma_handle = sg_dma_address(&chunk->mem[i]) +
-						dma_offset;
-				dma_offset -= sg_dma_len(&chunk->mem[i]);
+				if (len > dma_offset)
+					*dma_handle = dma_addr + dma_offset;
+				dma_offset -= len;
 			}
+
 			/*
 			 * DMA mapping can merge pages but not split them,
 			 * so if we found the page, dma_handle has already
 			 * been assigned to.
 			 */
-			if (chunk->mem[i].length > offset) {
-				page = sg_page(&chunk->mem[i]);
+			if (len > offset)
 				goto out;
-			}
-			offset -= chunk->mem[i].length;
+			offset -= len;
 		}
 	}
 
+	addr = NULL;
 out:
 	mutex_unlock(&table->mutex);
-	return page ? lowmem_page_address(page) + offset : NULL;
+	return addr ? addr + offset : NULL;
 }
 
 int mlx4_table_get_range(struct mlx4_dev *dev, struct mlx4_icm_table *table,
diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.h b/drivers/net/ethernet/mellanox/mlx4/icm.h
index c9169a490557..574f4bcdd090 100644
--- a/drivers/net/ethernet/mellanox/mlx4/icm.h
+++ b/drivers/net/ethernet/mellanox/mlx4/icm.h
@@ -47,11 +47,21 @@ enum {
 	MLX4_ICM_PAGE_SIZE	= 1 << MLX4_ICM_PAGE_SHIFT,
 };
 
+struct mlx4_icm_buf {
+	void			*addr;
+	size_t			size;
+	dma_addr_t		dma_addr;
+};
+
 struct mlx4_icm_chunk {
 	struct list_head	list;
 	int			npages;
 	int			nsg;
-	struct scatterlist	mem[MLX4_ICM_CHUNK_LEN];
+	int			coherent;
+	union {
+		struct scatterlist	sg[MLX4_ICM_CHUNK_LEN];
+		struct mlx4_icm_buf	buf[MLX4_ICM_CHUNK_LEN];
+	};
 };
 
 struct mlx4_icm {
@@ -114,12 +124,18 @@ static inline void mlx4_icm_next(struct mlx4_icm_iter *iter)
 
 static inline dma_addr_t mlx4_icm_addr(struct mlx4_icm_iter *iter)
 {
-	return sg_dma_address(&iter->chunk->mem[iter->page_idx]);
+	if (iter->chunk->coherent)
+		return iter->chunk->buf[iter->page_idx].dma_addr;
+	else
+		return sg_dma_address(&iter->chunk->sg[iter->page_idx]);
 }
 
 static inline unsigned long mlx4_icm_size(struct mlx4_icm_iter *iter)
 {
-	return sg_dma_len(&iter->chunk->mem[iter->page_idx]);
+	if (iter->chunk->coherent)
+		return iter->chunk->buf[iter->page_idx].size;
+	else
+		return sg_dma_len(&iter->chunk->sg[iter->page_idx]);
 }
 
 int mlx4_MAP_ICM_AUX(struct mlx4_dev *dev, struct mlx4_icm *icm);
-- 
2.19.2

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Jason Gunthorpe]

On Wed, Dec 19, 2018 at 11:20:31AM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> This patch solves a crash at the time of mlx4 driver unload or system
> shutdown. The crash occurs because dma_alloc_coherent() returns one
> value in mlx4_alloc_icm_coherent(), but a different value is passed to
> dma_free_coherent() in mlx4_free_icm_coherent(). In turn this is because
> when allocated, that pointer is passed to sg_set_buf() to record it,
> then when freed it is re-calculated by calling
> lowmem_page_address(sg_page()) which returns a different value. Solve
> this by recording the value that dma_alloc_coherent() returns, and
> passing this to dma_free_coherent().
> 
> This patch is roughly equivalent to commit 378efe798ecf ("RDMA/hns: Get
> rid of page operation after dma_alloc_coherent"). That patch was
> described as:
> 
> > In general, dma_alloc_coherent() returns a CPU virtual address and
> > a DMA address, and we have no guarantee that the underlying memory
> > even has an associated struct page at all.
> >
> > This patch gets rid of the page operation after dma_alloc_coherent,
> > and records the VA returned form dma_alloc_coherent in the struct
> > of hem in hns RoCE driver.
> 
> However, this patch reworks the code to store all information about
> coherent chunks separately from the sg list, since using sg lists for
> them doesn't make sense. Hence, the structure of this patch is quite
> different compared to the hns patch.
> 
> Based-on-code-from: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3:
> - Rework chunk data structure to store all data for coherent allocations
>   separately from the sg list. Code from Christoph Hellwig with fixes by
>   me. Notes:
>   - chunk->coherent is an int not a bool since checkpatch complains about
>     using bool in structs; see https://lkml.org/lkml/2017/11/21/384.

:( bool is much more readable and there is no performance concern in
this struct. I think checkpatch is overzealous here.

>   - chunk->coherent is used rather than chunk->table->coherent since the
>     table pointer isn't available when creating chunks. This duplicates
>     data, but simplifies the patch.
> v2:
> - Rework mlx4_table_find() to explicitly calculate the returned address
>   differently depending on wheter the table was allocated using
>   dma_alloc_coherent() or alloc_pages(), which in turn allows the
>   changes to mlx4_alloc_icm_pages() to be dropped.
> - Drop changes to mlx4_alloc/free_icm_pages. This path uses
>   pci_map_sg() which can re-write the sg list which in turn would cause
>   chunk->mem[] (the sg list) and chunk->buf[] to become inconsistent.
> - Enhance commit description.
> 
> Note: I've tested this patch in a downstream 4.14 based kernel (using
> ibping, ib_read_bw, and ib_write_bw), but can't test it in mainline
> since my system isn't supported there yet. I have compile-tested it in
> mainline at least, for ARM64.
> ---
>  drivers/net/ethernet/mellanox/mlx4/icm.c | 92 ++++++++++++++----------
>  drivers/net/ethernet/mellanox/mlx4/icm.h | 22 +++++-
>  2 files changed, 75 insertions(+), 39 deletions(-)

I think this needs an ack from the driver maintainers, and I gather
they are all on break for the next two weeks or so.

Lets revisit in January?

But at first glance it looks OK to me, though I would tidy the commit
message somewhat, your new leading paragraph fully explains the
problem, no need for the hns quote - and we can now see that the hns
should't have used a sgl either...

Jason

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Christoph Hellwig]

On Thu, Dec 20, 2018 at 10:43:18AM -0700, Jason Gunthorpe wrote:
> >   - chunk->coherent is an int not a bool since checkpatch complains about
> >     using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
> 
> :( bool is much more readable and there is no performance concern in
> this struct. I think checkpatch is overzealous here.

Yes.  Nevermind that this for bool vs bitfield.  A int is worse in
every respect in the criteria used in that mail.

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Bart Van Assche]

On Thu, 2018-12-20 at 18:44 +0100, Christoph Hellwig wrote:
> On Thu, Dec 20, 2018 at 10:43:18AM -0700, Jason Gunthorpe wrote:
> > >   - chunk->coherent is an int not a bool since checkpatch complains about
> > >     using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
> > 
> > :( bool is much more readable and there is no performance concern in
> > this struct. I think checkpatch is overzealous here.
> 
> Yes.  Nevermind that this for bool vs bitfield.  A int is worse in
> every respect in the criteria used in that mail.

(+Joe Perches)

Hi Joe,

This is the second time that I see that the checkpatch complaint about using
bool in a structure leads kernel contributors to a bad decision. Please consider
removing that warning from checkpatch.

Thanks,

Bart.

rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Joe Perches]

On Thu, 2018-12-20 at 09:49 -0800, Bart Van Assche wrote:
> On Thu, 2018-12-20 at 18:44 +0100, Christoph Hellwig wrote:
> > On Thu, Dec 20, 2018 at 10:43:18AM -0700, Jason Gunthorpe wrote:
> > > >   - chunk->coherent is an int not a bool since checkpatch complains about
> > > >     using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
> > > 
> > > :( bool is much more readable and there is no performance concern in
> > > this struct. I think checkpatch is overzealous here.
> > 
> > Yes.  Nevermind that this for bool vs bitfield.  A int is worse in
> > every respect in the criteria used in that mail.
> 
> (+Joe Perches)
> 
> Hi Joe,

Hi all.

> This is the second time that I see that the checkpatch complaint about using
> bool in a structure leads kernel contributors to a bad decision. Please consider
> removing that warning from checkpatch.

I agree it's not a very good message nor is bool use
of structure members a real problem except in very
few cases.

I think the message could either be restated and bool
members described as OK for unshared memory structures.

Right now this is the test:

# check for bool use in .h files
		if ($realfile =~ /\.h$/ &&
		    $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
			CHK("BOOL_MEMBER",
			    "Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" . $herecurr);
		}

What do you believe would be better message wording
or a perhaps an improved test?

btw: this can emit false positives for .h files with
static inline functions that declare bool automatics.

Linus' original email (https://lkml.org/lkml/2017/11/21/384) was:
---------------------------------------------------------------------
As others have already said, please don't use "bool" in structures at
all. It's an incredible waste of space, but it's also not entirely
clear what the type size even is. Usually a byte, but I don't think
there is any guarantee of it at all.

Use "bool" mainly as a return type from functions that return
true/false, and maybe as local variables in functions.

Maybe using single-bit bitfield with "bool" as the base type might
work, but the normal thing we tend to do is to just make sure the base
type is unsigned. It's a tiny bit more typing, but it's very
unambiguous what is going on, and you can specify the base type as you
wish and as it makes sense for packing etc.

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Andrew Morton]

On Thu, 20 Dec 2018 18:25:05 -0800 Joe Perches <joe@perches.com> wrote:

> On Thu, 2018-12-20 at 09:49 -0800, Bart Van Assche wrote:
> > On Thu, 2018-12-20 at 18:44 +0100, Christoph Hellwig wrote:
> > > On Thu, Dec 20, 2018 at 10:43:18AM -0700, Jason Gunthorpe wrote:
> > > > >   - chunk->coherent is an int not a bool since checkpatch complains about
> > > > >     using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
> > > > 
> > > > :( bool is much more readable and there is no performance concern in
> > > > this struct. I think checkpatch is overzealous here.
> > > 
> > > Yes.  Nevermind that this for bool vs bitfield.  A int is worse in
> > > every respect in the criteria used in that mail.
> > 
> > (+Joe Perches)
> > 
> > Hi Joe,
> 
> Hi all.
> 
> > This is the second time that I see that the checkpatch complaint about using
> > bool in a structure leads kernel contributors to a bad decision. Please consider
> > removing that warning from checkpatch.
> 
> I agree it's not a very good message nor is bool use
> of structure members a real problem except in very
> few cases.
> 
> I think the message could either be restated and bool
> members described as OK for unshared memory structures.
> 
> Right now this is the test:
> 
> # check for bool use in .h files
> 		if ($realfile =~ /\.h$/ &&
> 		    $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
> 			CHK("BOOL_MEMBER",
> 			    "Avoid using bool structure members because of possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" . $herecurr);

Probably better if this is in the tree. 
Documentation/process/coding-style.rst, perhaps.

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Jason Gunthorpe]

On Thu, Dec 20, 2018 at 06:25:05PM -0800, Joe Perches wrote:

> I agree it's not a very good message nor is bool use
> of structure members a real problem except in very
> few cases.

I think the issue is that the link it shows lacks the context of the
discussion thread - and the actual guidance here is more
complicated.

A discussion in coding style would probably be better.

- Don't put multiple true/false fields in a single struct (be it bool,
  u8, int, etc) - use bit-fields instead.
- Don't use bool if cache line layout matters, its size and alignment
  varies
- Do use bool to clearly express that the variable, parameter or
  structure member can only take on a true/false value.

.. and I also seem to remember there is a small performance downside
to bool stores in some ABIs as the compiler has to cannonize stores to
0 or 1? But this also eliminates certain bug classes..

Jason

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Joe Perches]

On Thu, 2018-12-20 at 20:31 -0700, Jason Gunthorpe wrote:
> On Thu, Dec 20, 2018 at 06:25:05PM -0800, Joe Perches wrote:
> 
> > I agree it's not a very good message nor is bool use
> > of structure members a real problem except in very
> > few cases.
> 
> I think the issue is that the link it shows lacks the context of the
> discussion thread - and the actual guidance here is more
> complicated.
> 
> A discussion in coding style would probably be better.

Perhaps so.

Care to submit a coding_style.rst patch or
improve the one below this?

> - Don't put multiple true/false fields in a single struct (be it bool,
>   u8, int, etc) - use bit-fields instead.

I'm not of the opinion this is always true.

rmw sometimes has a cost that bool does not.

Maybe 5 or more consecutive bools, but fewer
than that probably has no significant cost
when used consecutively in a struct.

Except maybe on the odd architecture. (alpha, etc)

> - Don't use bool if cache line layout matters, its size and alignment
>   varies

Completely right and that is the primary reason
to avoid bool use in structs IMO.

> - Do use bool to clearly express that the variable, parameter or
>   structure member can only take on a true/false value.

Yes absolutely, but that does conflict with the
first bullet point

> .. and I also seem to remember there is a small performance downside
> to bool stores in some ABIs as the compiler has to cannonize stores to
> 0 or 1? But this also eliminates certain bug classes..

Cannonize?  Canonicalize?
I think it's interesting when compilers go boom.

Anyway, both of those points are very true too.

cheers, Joe

Maybe this patch to coding-style.rst is a starting point?
---
 Documentation/process/coding-style.rst | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index b78dd680c038..752bead24d4a 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -921,7 +921,25 @@ result.  Typical examples would be functions that return pointers; they use
 NULL or the ERR_PTR mechanism to report failure.
 
 
-17) Don't re-invent the kernel macros
+17) Using bool
+--------------
+
+bool function return types are always fine to use whenever appropriate.
+bool structure members are more complicated.
+
+If memory utilization or alignment is a concern, please do not use bool
+structure members.  Do not use bool if cache line layout matters, its size
+and alignment varies based on the compiled architecture.
+
+Do use bool to clearly express that the variable, parameter or structure
+member can only take on a true/false value.
+
+There can be a small performance downside to bool stores in some ABIs as
+the compiler has to canonicalize stores to 0 or 1.  But doing so can also
+eliminates certain bug classes.
+
+
+18) Don't re-invent the kernel macros
 -------------------------------------
 
 The header file include/linux/kernel.h contains a number of macros that
@@ -944,7 +962,7 @@ need them.  Feel free to peruse that header file to see what else is already
 defined that you shouldn't reproduce in your code.
 
 
-18) Editor modelines and other cruft
+19) Editor modelines and other cruft
 ------------------------------------
 
 Some editors can interpret configuration information embedded in source files,
@@ -978,7 +996,7 @@ own custom mode, or may have some other magic method for making indentation
 work correctly.
 
 
-19) Inline assembly
+20) Inline assembly
 -------------------
 
 In architecture-specific code, you may need to use inline assembly to interface
@@ -1010,7 +1028,7 @@ the next instruction in the assembly output:
 	     : /* outputs */ : /* inputs */ : /* clobbers */);
 
 
-20) Conditional Compilation
+21) Conditional Compilation
 ---------------------------
 
 Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Jason Gunthorpe]

On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
> Care to submit a coding_style.rst patch or
> improve the one below this?

I took yours and revised it a little bit. I spent some time looking at
assembly and decided to drop the performance note, the number of cases
that run into overhead seems pretty small and probably already
requires !! to be correct. There is also an equally unlikely gain, ie
'if (a & b)' optimizes a tiny bit better for bool types.

I also added a small intro on bool, as I know some people are
unfamiliar with C11 _Bool and might think bool is just '#define bool
u8'

(for those added to the cc) I'm looking at cases, like the patch that
spawned this, where the struct has a single bool and no performance
considerations. As CH said, seeing that get converted to int due to
checkpatch is worse than having used bool. Using u8 won't make this
struct smaller or faster.

Cheers,
Jason

>From c5e2c887f6326e1c4369876f39996417da5e90cc Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Fri, 21 Dec 2018 16:29:22 -0700
Subject: [PATCH] coding-style: Clarify the expectations around bool

There has been some confusion since checkpatch started warning about bool
use in structures, and people have been avoiding using it.

Many people feel there is still a legitimate place for bool in structures,
so provide some guidance on bool usage derived from the entire thread that
spawned the checkpatch warning.

Link: https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 Documentation/process/coding-style.rst | 33 ++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 4e7c0a1c427a9a..efdb1d32035fe1 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -918,7 +918,32 @@ result.  Typical examples would be functions that return pointers; they use
 NULL or the ERR_PTR mechanism to report failure.
 
 
-17) Don't re-invent the kernel macros
+17) Using bool
+--------------
+
+The Linux kernel uses the C11 standard for the bool type. bool values can only
+evaluate to 0 or 1, and implicit or explicit conversion to bool automatically
+converts the value to true or false. When using bool types the !! construction
+is not needed, which eliminates a class of bugs.
+
+When working with bool values the true and false labels should be used instead
+of 0 and 1.
+
+bool function return types, arguments and stack variables are always fine to
+use whenever appropriate. Use of bool is encouraged to improve readability and
+is often a better option than 'int' for storing boolean values.
+
+Do not use bool if cache line layout or size of the value matters, its size
+and alignment varies based on the compiled architecture. Structures that are
+optimized for alignment and size should not use bool.
+
+If a structure has many true/false values, consider consolidating them into a
+bitfield with 1 bit members, or using an appropriate fixed width type, such as
+u8.
+
+Otherwise limited use of bool in structures does improve readability.
+
+18) Don't re-invent the kernel macros
 -------------------------------------
 
 The header file include/linux/kernel.h contains a number of macros that
@@ -941,7 +966,7 @@ need them.  Feel free to peruse that header file to see what else is already
 defined that you shouldn't reproduce in your code.
 
 
-18) Editor modelines and other cruft
+19) Editor modelines and other cruft
 ------------------------------------
 
 Some editors can interpret configuration information embedded in source files,
@@ -975,7 +1000,7 @@ own custom mode, or may have some other magic method for making indentation
 work correctly.
 
 
-19) Inline assembly
+20) Inline assembly
 -------------------
 
 In architecture-specific code, you may need to use inline assembly to interface
@@ -1007,7 +1032,7 @@ the next instruction in the assembly output:
 	     : /* outputs */ : /* inputs */ : /* clobbers */);
 
 
-20) Conditional Compilation
+21) Conditional Compilation
 ---------------------------
 
 Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
-- 
2.19.2

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Jason Gunthorpe]

On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
> Care to submit a coding_style.rst patch or
> improve the one below this?

I took yours and revised it a little bit. I spent some time looking at
assembly and decided to drop the performance note, the number of cases
that run into overhead seems pretty small and probably already
requires !! to be correct. There is also an equally unlikely gain, ie
'if (a & b)' optimizes a tiny bit better for bool types.

I also added a small intro on bool, as I know some people are
unfamiliar with C11 _Bool and might think bool is just '#define bool
u8'

(for those added to the cc) I'm looking at cases, like the patch that
spawned this, where the struct has a single bool and no performance
considerations. As CH said, seeing that get converted to int due to
checkpatch is worse than having used bool. Using u8 won't make this
struct smaller or faster.

Cheers,
Jason

From c5e2c887f6326e1c4369876f39996417da5e90cc Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Fri, 21 Dec 2018 16:29:22 -0700
Subject: [PATCH] coding-style: Clarify the expectations around bool

There has been some confusion since checkpatch started warning about bool
use in structures, and people have been avoiding using it.

Many people feel there is still a legitimate place for bool in structures,
so provide some guidance on bool usage derived from the entire thread that
spawned the checkpatch warning.

Link: https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 Documentation/process/coding-style.rst | 33 ++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 4e7c0a1c427a9a..efdb1d32035fe1 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -918,7 +918,32 @@ result.  Typical examples would be functions that return pointers; they use
 NULL or the ERR_PTR mechanism to report failure.
 
 
-17) Don't re-invent the kernel macros
+17) Using bool
+--------------
+
+The Linux kernel uses the C11 standard for the bool type. bool values can only
+evaluate to 0 or 1, and implicit or explicit conversion to bool automatically
+converts the value to true or false. When using bool types the !! construction
+is not needed, which eliminates a class of bugs.
+
+When working with bool values the true and false labels should be used instead
+of 0 and 1.
+
+bool function return types, arguments and stack variables are always fine to
+use whenever appropriate. Use of bool is encouraged to improve readability and
+is often a better option than 'int' for storing boolean values.
+
+Do not use bool if cache line layout or size of the value matters, its size
+and alignment varies based on the compiled architecture. Structures that are
+optimized for alignment and size should not use bool.
+
+If a structure has many true/false values, consider consolidating them into a
+bitfield with 1 bit members, or using an appropriate fixed width type, such as
+u8.
+
+Otherwise limited use of bool in structures does improve readability.
+
+18) Don't re-invent the kernel macros
 -------------------------------------
 
 The header file include/linux/kernel.h contains a number of macros that
@@ -941,7 +966,7 @@ need them.  Feel free to peruse that header file to see what else is already
 defined that you shouldn't reproduce in your code.
 
 
-18) Editor modelines and other cruft
+19) Editor modelines and other cruft
 ------------------------------------
 
 Some editors can interpret configuration information embedded in source files,
@@ -975,7 +1000,7 @@ own custom mode, or may have some other magic method for making indentation
 work correctly.
 
 
-19) Inline assembly
+20) Inline assembly
 -------------------
 
 In architecture-specific code, you may need to use inline assembly to interface
@@ -1007,7 +1032,7 @@ the next instruction in the assembly output:
 	     : /* outputs */ : /* inputs */ : /* clobbers */);
 
 
-20) Conditional Compilation
+21) Conditional Compilation
 ---------------------------
 
 Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
-- 
2.19.2

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Gal Pressman]

On 22-Dec-18 01:52, Jason Gunthorpe wrote:
> On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
>> Care to submit a coding_style.rst patch or
>> improve the one below this?
> 
> I took yours and revised it a little bit. I spent some time looking at
> assembly and decided to drop the performance note, the number of cases
> that run into overhead seems pretty small and probably already
> requires !! to be correct. There is also an equally unlikely gain, ie
> 'if (a & b)' optimizes a tiny bit better for bool types.
> 
> I also added a small intro on bool, as I know some people are
> unfamiliar with C11 _Bool and might think bool is just '#define bool
> u8'
> 
> (for those added to the cc) I'm looking at cases, like the patch that
> spawned this, where the struct has a single bool and no performance
> considerations. As CH said, seeing that get converted to int due to
> checkpatch is worse than having used bool. Using u8 won't make this
> struct smaller or faster.
> 
> Cheers,
> Jason
> 
> From c5e2c887f6326e1c4369876f39996417da5e90cc Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@mellanox.com>
> Date: Fri, 21 Dec 2018 16:29:22 -0700
> Subject: [PATCH] coding-style: Clarify the expectations around bool
> 
> There has been some confusion since checkpatch started warning about bool
> use in structures, and people have been avoiding using it.
> 
> Many people feel there is still a legitimate place for bool in structures,
> so provide some guidance on bool usage derived from the entire thread that
> spawned the checkpatch warning.

Since a "Using bool" section is added, perhaps it's worth documenting the bool
usage as a function parameter [1]?

[1] https://www.spinics.net/lists/linux-rdma/msg72336.html

> 
> Link: https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
> Signed-off-by: Joe Perches <joe@perches.com>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  Documentation/process/coding-style.rst | 33 ++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 4e7c0a1c427a9a..efdb1d32035fe1 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -918,7 +918,32 @@ result.  Typical examples would be functions that return pointers; they use
>  NULL or the ERR_PTR mechanism to report failure.
>  
>  
> -17) Don't re-invent the kernel macros
> +17) Using bool
> +--------------
> +
> +The Linux kernel uses the C11 standard for the bool type. bool values can only
> +evaluate to 0 or 1, and implicit or explicit conversion to bool automatically
> +converts the value to true or false. When using bool types the !! construction
> +is not needed, which eliminates a class of bugs.
> +
> +When working with bool values the true and false labels should be used instead
> +of 0 and 1.
> +
> +bool function return types, arguments and stack variables are always fine to
> +use whenever appropriate. Use of bool is encouraged to improve readability and
> +is often a better option than 'int' for storing boolean values.
> +
> +Do not use bool if cache line layout or size of the value matters, its size
> +and alignment varies based on the compiled architecture. Structures that are
> +optimized for alignment and size should not use bool.
> +
> +If a structure has many true/false values, consider consolidating them into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> +u8.
> +
> +Otherwise limited use of bool in structures does improve readability.
> +
> +18) Don't re-invent the kernel macros
>  -------------------------------------
>  
>  The header file include/linux/kernel.h contains a number of macros that
> @@ -941,7 +966,7 @@ need them.  Feel free to peruse that header file to see what else is already
>  defined that you shouldn't reproduce in your code.
>  
>  
> -18) Editor modelines and other cruft
> +19) Editor modelines and other cruft
>  ------------------------------------
>  
>  Some editors can interpret configuration information embedded in source files,
> @@ -975,7 +1000,7 @@ own custom mode, or may have some other magic method for making indentation
>  work correctly.
>  
>  
> -19) Inline assembly
> +20) Inline assembly
>  -------------------
>  
>  In architecture-specific code, you may need to use inline assembly to interface
> @@ -1007,7 +1032,7 @@ the next instruction in the assembly output:
>  	     : /* outputs */ : /* inputs */ : /* clobbers */);
>  
>  
> -20) Conditional Compilation
> +21) Conditional Compilation
>  ---------------------------
>  
>  Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> 

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Gal Pressman]

On 22-Dec-18 01:52, Jason Gunthorpe wrote:
> On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
>> Care to submit a coding_style.rst patch or
>> improve the one below this?
> 
> I took yours and revised it a little bit. I spent some time looking at
> assembly and decided to drop the performance note, the number of cases
> that run into overhead seems pretty small and probably already
> requires !! to be correct. There is also an equally unlikely gain, ie
> 'if (a & b)' optimizes a tiny bit better for bool types.
> 
> I also added a small intro on bool, as I know some people are
> unfamiliar with C11 _Bool and might think bool is just '#define bool
> u8'
> 
> (for those added to the cc) I'm looking at cases, like the patch that
> spawned this, where the struct has a single bool and no performance
> considerations. As CH said, seeing that get converted to int due to
> checkpatch is worse than having used bool. Using u8 won't make this
> struct smaller or faster.
> 
> Cheers,
> Jason
> 
> From c5e2c887f6326e1c4369876f39996417da5e90cc Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgg@mellanox.com>
> Date: Fri, 21 Dec 2018 16:29:22 -0700
> Subject: [PATCH] coding-style: Clarify the expectations around bool
> 
> There has been some confusion since checkpatch started warning about bool
> use in structures, and people have been avoiding using it.
> 
> Many people feel there is still a legitimate place for bool in structures,
> so provide some guidance on bool usage derived from the entire thread that
> spawned the checkpatch warning.

Since a "Using bool" section is added, perhaps it's worth documenting the bool
usage as a function parameter [1]?

[1] https://www.spinics.net/lists/linux-rdma/msg72336.html

> 
> Link: https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
> Signed-off-by: Joe Perches <joe@perches.com>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  Documentation/process/coding-style.rst | 33 ++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 4e7c0a1c427a9a..efdb1d32035fe1 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -918,7 +918,32 @@ result.  Typical examples would be functions that return pointers; they use
>  NULL or the ERR_PTR mechanism to report failure.
>  
>  
> -17) Don't re-invent the kernel macros
> +17) Using bool
> +--------------
> +
> +The Linux kernel uses the C11 standard for the bool type. bool values can only
> +evaluate to 0 or 1, and implicit or explicit conversion to bool automatically
> +converts the value to true or false. When using bool types the !! construction
> +is not needed, which eliminates a class of bugs.
> +
> +When working with bool values the true and false labels should be used instead
> +of 0 and 1.
> +
> +bool function return types, arguments and stack variables are always fine to
> +use whenever appropriate. Use of bool is encouraged to improve readability and
> +is often a better option than 'int' for storing boolean values.
> +
> +Do not use bool if cache line layout or size of the value matters, its size
> +and alignment varies based on the compiled architecture. Structures that are
> +optimized for alignment and size should not use bool.
> +
> +If a structure has many true/false values, consider consolidating them into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> +u8.
> +
> +Otherwise limited use of bool in structures does improve readability.
> +
> +18) Don't re-invent the kernel macros
>  -------------------------------------
>  
>  The header file include/linux/kernel.h contains a number of macros that
> @@ -941,7 +966,7 @@ need them.  Feel free to peruse that header file to see what else is already
>  defined that you shouldn't reproduce in your code.
>  
>  
> -18) Editor modelines and other cruft
> +19) Editor modelines and other cruft
>  ------------------------------------
>  
>  Some editors can interpret configuration information embedded in source files,
> @@ -975,7 +1000,7 @@ own custom mode, or may have some other magic method for making indentation
>  work correctly.
>  
>  
> -19) Inline assembly
> +20) Inline assembly
>  -------------------
>  
>  In architecture-specific code, you may need to use inline assembly to interface
> @@ -1007,7 +1032,7 @@ the next instruction in the assembly output:
>  	     : /* outputs */ : /* inputs */ : /* clobbers */);
>  
>  
> -20) Conditional Compilation
> +21) Conditional Compilation
>  ---------------------------
>  
>  Wherever possible, don't use preprocessor conditionals (#if, #ifdef) in .c
> 

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Al Viro]

On Sun, Dec 23, 2018 at 06:42:20PM +0200, Gal Pressman wrote:
> > -17) Don't re-invent the kernel macros
> > +17) Using bool
> > +--------------
> > +
> > +The Linux kernel uses the C11 standard for the bool type. bool values can only

C99, surely?

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Jason Gunthorpe]

On Sun, Dec 23, 2018 at 04:53:12PM +0000, Al Viro wrote:
> On Sun, Dec 23, 2018 at 06:42:20PM +0200, Gal Pressman wrote:
> > > -17) Don't re-invent the kernel macros
> > > +17) Using bool
> > > +--------------
> > > +
> > > +The Linux kernel uses the C11 standard for the bool type. bool values can only
> 
> C99, surely?

Oops, yes, that is right

Jason

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Jason Gunthorpe]

On Sun, Dec 23, 2018 at 06:42:20PM +0200, Gal Pressman wrote:
> On 22-Dec-18 01:52, Jason Gunthorpe wrote:
> > On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
> >> Care to submit a coding_style.rst patch or
> >> improve the one below this?
> > 
> > I took yours and revised it a little bit. I spent some time looking at
> > assembly and decided to drop the performance note, the number of cases
> > that run into overhead seems pretty small and probably already
> > requires !! to be correct. There is also an equally unlikely gain, ie
> > 'if (a & b)' optimizes a tiny bit better for bool types.
> > 
> > I also added a small intro on bool, as I know some people are
> > unfamiliar with C11 _Bool and might think bool is just '#define bool
> > u8'
> > 
> > (for those added to the cc) I'm looking at cases, like the patch that
> > spawned this, where the struct has a single bool and no performance
> > considerations. As CH said, seeing that get converted to int due to
> > checkpatch is worse than having used bool. Using u8 won't make this
> > struct smaller or faster.
> > 
> 
> Since a "Using bool" section is added, perhaps it's worth documenting the bool
> usage as a function parameter [1]?
> 
> [1] https://www.spinics.net/lists/linux-rdma/msg72336.html

I'm not really sure how to express that as something concrete.. That
specific case clearly called out for a flags as it was a widely used
API - maybe less spread out cases like static functions or something
are OK??

Jason

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Gal Pressman]

On 25-Dec-18 01:12, Jason Gunthorpe wrote:
> On Sun, Dec 23, 2018 at 06:42:20PM +0200, Gal Pressman wrote:
>> On 22-Dec-18 01:52, Jason Gunthorpe wrote:
>>> On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
>>>> Care to submit a coding_style.rst patch or
>>>> improve the one below this?
>>>
>>> I took yours and revised it a little bit. I spent some time looking at
>>> assembly and decided to drop the performance note, the number of cases
>>> that run into overhead seems pretty small and probably already
>>> requires !! to be correct. There is also an equally unlikely gain, ie
>>> 'if (a & b)' optimizes a tiny bit better for bool types.
>>>
>>> I also added a small intro on bool, as I know some people are
>>> unfamiliar with C11 _Bool and might think bool is just '#define bool
>>> u8'
>>>
>>> (for those added to the cc) I'm looking at cases, like the patch that
>>> spawned this, where the struct has a single bool and no performance
>>> considerations. As CH said, seeing that get converted to int due to
>>> checkpatch is worse than having used bool. Using u8 won't make this
>>> struct smaller or faster.
>>>
>>
>> Since a "Using bool" section is added, perhaps it's worth documenting the bool
>> usage as a function parameter [1]?
>>
>> [1] https://www.spinics.net/lists/linux-rdma/msg72336.html
> 
> I'm not really sure how to express that as something concrete.. That
> specific case clearly called out for a flags as it was a widely used
> API - maybe less spread out cases like static functions or something
> are OK??
> 
> Jason
> 

Sounds reasonable, sometimes adding flags and enum for a single bool function
parameter is a bit too much IMO. For a widely used API it makes sense.

Re: rfc: bool structure members (was Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent)

[Gal Pressman]

On 25-Dec-18 01:12, Jason Gunthorpe wrote:
> On Sun, Dec 23, 2018 at 06:42:20PM +0200, Gal Pressman wrote:
>> On 22-Dec-18 01:52, Jason Gunthorpe wrote:
>>> On Thu, Dec 20, 2018 at 09:12:43PM -0800, Joe Perches wrote:
>>>> Care to submit a coding_style.rst patch or
>>>> improve the one below this?
>>>
>>> I took yours and revised it a little bit. I spent some time looking at
>>> assembly and decided to drop the performance note, the number of cases
>>> that run into overhead seems pretty small and probably already
>>> requires !! to be correct. There is also an equally unlikely gain, ie
>>> 'if (a & b)' optimizes a tiny bit better for bool types.
>>>
>>> I also added a small intro on bool, as I know some people are
>>> unfamiliar with C11 _Bool and might think bool is just '#define bool
>>> u8'
>>>
>>> (for those added to the cc) I'm looking at cases, like the patch that
>>> spawned this, where the struct has a single bool and no performance
>>> considerations. As CH said, seeing that get converted to int due to
>>> checkpatch is worse than having used bool. Using u8 won't make this
>>> struct smaller or faster.
>>>
>>
>> Since a "Using bool" section is added, perhaps it's worth documenting the bool
>> usage as a function parameter [1]?
>>
>> [1] https://www.spinics.net/lists/linux-rdma/msg72336.html
> 
> I'm not really sure how to express that as something concrete.. That
> specific case clearly called out for a flags as it was a widely used
> API - maybe less spread out cases like static functions or something
> are OK??
> 
> Jason
> 

Sounds reasonable, sometimes adding flags and enum for a single bool function
parameter is a bit too much IMO. For a widely used API it makes sense.

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Stephen Warren]

On 12/20/18 10:43 AM, Jason Gunthorpe wrote:
> On Wed, Dec 19, 2018 at 11:20:31AM -0700, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This patch solves a crash at the time of mlx4 driver unload or system
>> shutdown. The crash occurs because dma_alloc_coherent() returns one
>> value in mlx4_alloc_icm_coherent(), but a different value is passed to
>> dma_free_coherent() in mlx4_free_icm_coherent(). In turn this is because
>> when allocated, that pointer is passed to sg_set_buf() to record it,
>> then when freed it is re-calculated by calling
>> lowmem_page_address(sg_page()) which returns a different value. Solve
>> this by recording the value that dma_alloc_coherent() returns, and
>> passing this to dma_free_coherent().
>>
>> This patch is roughly equivalent to commit 378efe798ecf ("RDMA/hns: Get
>> rid of page operation after dma_alloc_coherent"). That patch was
>> described as:
>>
>>> In general, dma_alloc_coherent() returns a CPU virtual address and
>>> a DMA address, and we have no guarantee that the underlying memory
>>> even has an associated struct page at all.
>>>
>>> This patch gets rid of the page operation after dma_alloc_coherent,
>>> and records the VA returned form dma_alloc_coherent in the struct
>>> of hem in hns RoCE driver.
>>
>> However, this patch reworks the code to store all information about
>> coherent chunks separately from the sg list, since using sg lists for
>> them doesn't make sense. Hence, the structure of this patch is quite
>> different compared to the hns patch.
>>
>> Based-on-code-from: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> v3:
>> - Rework chunk data structure to store all data for coherent allocations
>>    separately from the sg list. Code from Christoph Hellwig with fixes by
>>    me. Notes:
>>    - chunk->coherent is an int not a bool since checkpatch complains about
>>      using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
> 
> :( bool is much more readable and there is no performance concern in
> this struct. I think checkpatch is overzealous here.
> 
>>    - chunk->coherent is used rather than chunk->table->coherent since the
>>      table pointer isn't available when creating chunks. This duplicates
>>      data, but simplifies the patch.
>> v2:
>> - Rework mlx4_table_find() to explicitly calculate the returned address
>>    differently depending on wheter the table was allocated using
>>    dma_alloc_coherent() or alloc_pages(), which in turn allows the
>>    changes to mlx4_alloc_icm_pages() to be dropped.
>> - Drop changes to mlx4_alloc/free_icm_pages. This path uses
>>    pci_map_sg() which can re-write the sg list which in turn would cause
>>    chunk->mem[] (the sg list) and chunk->buf[] to become inconsistent.
>> - Enhance commit description.
>>
>> Note: I've tested this patch in a downstream 4.14 based kernel (using
>> ibping, ib_read_bw, and ib_write_bw), but can't test it in mainline
>> since my system isn't supported there yet. I have compile-tested it in
>> mainline at least, for ARM64.
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/icm.c | 92 ++++++++++++++----------
>>   drivers/net/ethernet/mellanox/mlx4/icm.h | 22 +++++-
>>   2 files changed, 75 insertions(+), 39 deletions(-)
> 
> I think this needs an ack from the driver maintainers, and I gather
> they are all on break for the next two weeks or so.
> 
> Lets revisit in January?
> 
> But at first glance it looks OK to me, though I would tidy the commit
> message somewhat, your new leading paragraph fully explains the
> problem, no need for the hns quote - and we can now see that the hns
> should't have used a sgl either...

Tariq, I assume you're back from vacation now/soon. What do you think of 
this patch, aside from the bool-vs-int issue that I plan to fix up soon. 
Thanks.

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Christoph Hellwig]

Btw, one other little request if you resend this: the mlx5 icm code
touched here oddly enough uses the deprecated pci_map_sg/pci_unmap_sg
instead of the preferred dma_map_sg/dma_unmap_sg despite otherwise
using the generic DMA API.  Can you throw in a little cleanup patch
for that into your series?

Re: [PATCH V3] net/mlx4: Get rid of page operation after dma_alloc_coherent

[Tariq Toukan]

On 1/2/2019 6:29 PM, Stephen Warren wrote:
> On 12/20/18 10:43 AM, Jason Gunthorpe wrote:
>> On Wed, Dec 19, 2018 at 11:20:31AM -0700, Stephen Warren wrote:
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> This patch solves a crash at the time of mlx4 driver unload or system
>>> shutdown. The crash occurs because dma_alloc_coherent() returns one
>>> value in mlx4_alloc_icm_coherent(), but a different value is passed to
>>> dma_free_coherent() in mlx4_free_icm_coherent(). In turn this is because
>>> when allocated, that pointer is passed to sg_set_buf() to record it,
>>> then when freed it is re-calculated by calling
>>> lowmem_page_address(sg_page()) which returns a different value. Solve
>>> this by recording the value that dma_alloc_coherent() returns, and
>>> passing this to dma_free_coherent().
>>>
>>> This patch is roughly equivalent to commit 378efe798ecf ("RDMA/hns: Get
>>> rid of page operation after dma_alloc_coherent"). That patch was
>>> described as:
>>>
>>>> In general, dma_alloc_coherent() returns a CPU virtual address and
>>>> a DMA address, and we have no guarantee that the underlying memory
>>>> even has an associated struct page at all.
>>>>
>>>> This patch gets rid of the page operation after dma_alloc_coherent,
>>>> and records the VA returned form dma_alloc_coherent in the struct
>>>> of hem in hns RoCE driver.
>>>
>>> However, this patch reworks the code to store all information about
>>> coherent chunks separately from the sg list, since using sg lists for
>>> them doesn't make sense. Hence, the structure of this patch is quite
>>> different compared to the hns patch.
>>>
>>> Based-on-code-from: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>> ---
>>> v3:
>>> - Rework chunk data structure to store all data for coherent allocations
>>>    separately from the sg list. Code from Christoph Hellwig with 
>>> fixes by
>>>    me. Notes:
>>>    - chunk->coherent is an int not a bool since checkpatch complains 
>>> about
>>>      using bool in structs; see https://lkml.org/lkml/2017/11/21/384.
>>
>> :( bool is much more readable and there is no performance concern in
>> this struct. I think checkpatch is overzealous here.
>>
>>>    - chunk->coherent is used rather than chunk->table->coherent since 
>>> the
>>>      table pointer isn't available when creating chunks. This duplicates
>>>      data, but simplifies the patch.
>>> v2:
>>> - Rework mlx4_table_find() to explicitly calculate the returned address
>>>    differently depending on wheter the table was allocated using
>>>    dma_alloc_coherent() or alloc_pages(), which in turn allows the
>>>    changes to mlx4_alloc_icm_pages() to be dropped.
>>> - Drop changes to mlx4_alloc/free_icm_pages. This path uses
>>>    pci_map_sg() which can re-write the sg list which in turn would cause
>>>    chunk->mem[] (the sg list) and chunk->buf[] to become inconsistent.
>>> - Enhance commit description.
>>>
>>> Note: I've tested this patch in a downstream 4.14 based kernel (using
>>> ibping, ib_read_bw, and ib_write_bw), but can't test it in mainline
>>> since my system isn't supported there yet. I have compile-tested it in
>>> mainline at least, for ARM64.
>>> ---
>>>   drivers/net/ethernet/mellanox/mlx4/icm.c | 92 ++++++++++++++----------
>>>   drivers/net/ethernet/mellanox/mlx4/icm.h | 22 +++++-
>>>   2 files changed, 75 insertions(+), 39 deletions(-)
>>
>> I think this needs an ack from the driver maintainers, and I gather
>> they are all on break for the next two weeks or so.
>>
>> Lets revisit in January?
>>
>> But at first glance it looks OK to me, though I would tidy the commit
>> message somewhat, your new leading paragraph fully explains the
>> problem, no need for the hns quote - and we can now see that the hns
>> should't have used a sgl either...
> 
> Tariq, I assume you're back from vacation now/soon. What do you think of 
> this patch, aside from the bool-vs-int issue that I plan to fix up soon. 
> Thanks.

Hi Stephen,

Thanks for your patch.
It looks good to me.

Tariq