* [PATCH 87/87] ethernet: mlx4: remove memset after dma_alloc_coherent
From: Fuqian Huang @ 2019-06-27 17:42 UTC (permalink / raw)
Cc: Fuqian Huang, Tariq Toukan, David S. Miller, netdev, linux-rdma,
linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/net/ethernet/mellanox/mlx4/eq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index a5be27772b8e..c790a5fcea73 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -1013,8 +1013,6 @@ static int mlx4_create_eq(struct mlx4_dev *dev, int nent,
dma_list[i] = t;
eq->page_list[i].map = t;
-
- memset(eq->page_list[i].buf, 0, PAGE_SIZE);
}
eq->eqn = mlx4_bitmap_alloc(&priv->eq_table.bitmap);
--
2.11.0
^ permalink raw reply related
* [PATCH 28/87] infiniband: ulp: remove memset after vzalloc in ipoib_cm.c
From: Fuqian Huang @ 2019-06-27 17:38 UTC (permalink / raw)
Cc: Fuqian Huang, Doug Ledford, Jason Gunthorpe, Leon Romanovsky,
Kamal Heib, Feras Daoud, Erez Shitrit, Bart Van Assche,
Aaron Knister, Denis Drozdov, Parav Pandit, linux-rdma,
linux-kernel
vzalloc has already zeroed the memory.
So memset is unneeded.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/ulp/ipoib/ipoib_cm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index aa9dcfc36cd3..c59e00a0881f 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -1153,7 +1153,6 @@ static int ipoib_cm_tx_init(struct ipoib_cm_tx *p, u32 qpn,
ret = -ENOMEM;
goto err_tx;
}
- memset(p->tx_ring, 0, ipoib_sendq_size * sizeof(*p->tx_ring));
p->qp = ipoib_cm_create_tx_qp(p->dev, p);
memalloc_noio_restore(noio_flag);
--
2.11.0
^ permalink raw reply related
* [PATCH 27/87] infiniband: ocrdma: remove memset after dma_alloc_coherent in ocrdma_hw.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Selvin Xavier, Devesh Sharma, Doug Ledford,
Jason Gunthorpe, Leon Romanovsky, Luis Chamberlain,
Bart Van Assche, Parav Pandit, linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
index 5127e2ea4bdd..6e07712eb3ed 100644
--- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c
@@ -1351,7 +1351,6 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev)
mqe->u.nonemb_req.sge[0].pa_hi = (u32) upper_32_bits(dma.pa);
mqe->u.nonemb_req.sge[0].len = dma.size;
- memset(dma.va, 0, dma.size);
ocrdma_init_mch((struct ocrdma_mbx_hdr *)dma.va,
OCRDMA_CMD_GET_CTRL_ATTRIBUTES,
OCRDMA_SUBSYS_COMMON,
@@ -1690,7 +1689,6 @@ static int ocrdma_mbx_create_ah_tbl(struct ocrdma_dev *dev)
goto mem_err_ah;
dev->av_tbl.pa = pa;
dev->av_tbl.num_ah = max_ah;
- memset(dev->av_tbl.va, 0, dev->av_tbl.size);
pbes = (struct ocrdma_pbe *)dev->av_tbl.pbl.va;
for (i = 0; i < dev->av_tbl.size / OCRDMA_MIN_Q_PAGE_SIZE; i++) {
@@ -2905,7 +2903,6 @@ static int ocrdma_mbx_get_dcbx_config(struct ocrdma_dev *dev, u32 ptype,
mqe_sge->pa_hi = (u32) upper_32_bits(pa);
mqe_sge->len = cmd.hdr.pyld_len;
- memset(req, 0, sizeof(struct ocrdma_get_dcbx_cfg_req));
ocrdma_init_mch(&req->hdr, OCRDMA_CMD_GET_DCBX_CONFIG,
OCRDMA_SUBSYS_DCBX, cmd.hdr.pyld_len);
req->param_type = ptype;
--
2.11.0
^ permalink raw reply related
* [PATCH 26/87] infiniband: nes: remove memset after pci_alloc_consistent in nes_verbs.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Faisal Latif, Doug Ledford, Jason Gunthorpe,
linux-rdma, linux-kernel
pci_alloc_consistent calls dma_alloc_coherent directly.
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/nes/nes_verbs.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/hw/nes/nes_verbs.c b/drivers/infiniband/hw/nes/nes_verbs.c
index 49024326a518..534f978f1a58 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -828,7 +828,6 @@ static int nes_setup_virt_qp(struct nes_qp *nesqp, struct nes_pbl *nespbl,
kunmap(nesqp->page);
return -ENOMEM;
}
- memset(nesqp->pbl_vbase, 0, 256);
/* fill in the page address in the pbl buffer.. */
tpbl = pblbuffer + 16;
pbl = (__le64 *)nespbl->pbl_vbase;
@@ -898,8 +897,6 @@ static int nes_setup_mmap_qp(struct nes_qp *nesqp, struct nes_vnic *nesvnic,
"host descriptor rings located @ %p (pa = 0x%08lX.) size = %u.\n",
mem, (unsigned long)nesqp->hwqp.sq_pbase, nesqp->qp_mem_size);
- memset(mem, 0, nesqp->qp_mem_size);
-
nesqp->hwqp.sq_vbase = mem;
mem += sizeof(struct nes_hw_qp_wqe) * sq_size;
--
2.11.0
^ permalink raw reply related
* [PATCH 25/87] infiniband: mthca: remove memset after dma_alloc_coherent in mthca_allocator.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Doug Ledford, Jason Gunthorpe, linux-rdma,
linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/mthca/mthca_allocator.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_allocator.c b/drivers/infiniband/hw/mthca/mthca_allocator.c
index aaf10dd5364d..aef1d274a14e 100644
--- a/drivers/infiniband/hw/mthca/mthca_allocator.c
+++ b/drivers/infiniband/hw/mthca/mthca_allocator.c
@@ -214,8 +214,6 @@ int mthca_buf_alloc(struct mthca_dev *dev, int size, int max_direct,
dma_unmap_addr_set(&buf->direct, mapping, t);
- memset(buf->direct.buf, 0, size);
-
while (t & ((1 << shift) - 1)) {
--shift;
npages *= 2;
--
2.11.0
^ permalink raw reply related
* [PATCH 24/87] infiniband: hns: Remove memset after dma_alloc_coherent in hns_roce_hw_v2.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Lijun Ou, Wei Hu(Xavier), Doug Ledford,
Jason Gunthorpe, linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index b5392cb5b20f..a4a7c5962916 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1774,7 +1774,6 @@ static int hns_roce_init_link_table(struct hns_roce_dev *hr_dev,
goto err_alloc_buf_failed;
link_tbl->pg_list[i].map = t;
- memset(link_tbl->pg_list[i].buf, 0, buf_chk_sz);
entry[i].blk_ba0 = (t >> 12) & 0xffffffff;
roce_set_field(entry[i].blk_ba1_nxt_ptr,
@@ -5387,8 +5386,6 @@ static int hns_roce_mhop_alloc_eq(struct hns_roce_dev *hr_dev,
eq->cur_eqe_ba = eq->l0_dma;
eq->nxt_eqe_ba = 0;
- memset(eq->bt_l0, 0, eq->entries * eq->eqe_size);
-
return 0;
}
--
2.11.0
^ permalink raw reply related
* [PATCH 23/87] infiniband: hns: Remove memset after dma_alloc_coherent in hns_roce_hw_v1.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Lijun Ou, Wei Hu(Xavier), Doug Ledford,
Jason Gunthorpe, linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index e068a02122f5..36d9dcaaa8f9 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -4265,7 +4265,6 @@ static int hns_roce_v1_create_eq(struct hns_roce_dev *hr_dev,
}
eq->buf_list[i].map = tmp_dma_addr;
- memset(eq->buf_list[i].buf, 0, HNS_ROCE_BA_SIZE);
}
eq->cons_index = 0;
roce_set_field(tmp, ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_STATE_M,
--
2.11.0
^ permalink raw reply related
* [PATCH 22/87] infiniband: cxgb4: Remove call to memset after dma_alloc_coherent in qp.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Potnuri Bharat Teja, Doug Ledford, Jason Gunthorpe,
linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/cxgb4/qp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
index e92b9544357a..4882dcbb7d20 100644
--- a/drivers/infiniband/hw/cxgb4/qp.c
+++ b/drivers/infiniband/hw/cxgb4/qp.c
@@ -274,7 +274,6 @@ static int create_qp(struct c4iw_rdev *rdev, struct t4_wq *wq,
(unsigned long long)virt_to_phys(wq->sq.queue),
wq->rq.queue,
(unsigned long long)virt_to_phys(wq->rq.queue));
- memset(wq->rq.queue, 0, wq->rq.memsize);
dma_unmap_addr_set(&wq->rq, mapping, wq->rq.dma_addr);
}
--
2.11.0
^ permalink raw reply related
* [PATCH 21/87] infiniband: cxgb4: Remove call to memset after dma_alloc_coherent in cq.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Potnuri Bharat Teja, Doug Ledford, Jason Gunthorpe,
linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/cxgb4/cq.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index 52ce586621c6..fcd161e3495b 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -104,7 +104,6 @@ static int create_cq(struct c4iw_rdev *rdev, struct t4_cq *cq,
goto err3;
}
dma_unmap_addr_set(cq, mapping, cq->dma_addr);
- memset(cq->queue, 0, cq->memsize);
if (user && ucontext->is_32b_cqe) {
cq->qp_errp = &((struct t4_status_page *)
--
2.11.0
^ permalink raw reply related
* [PATCH 20/87] infiniband: cxgb3: Remove call to memset after dma_alloc_coherent in cxio_hal.c
From: Fuqian Huang @ 2019-06-27 17:37 UTC (permalink / raw)
Cc: Fuqian Huang, Potnuri Bharat Teja, Doug Ledford, Jason Gunthorpe,
linux-rdma, linux-kernel
In commit af7ddd8a627c
("Merge tag 'dma-mapping-4.21' of git://git.infradead.org/users/hch/dma-mapping"),
dma_alloc_coherent has already zeroed the memory.
So memset is not needed.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/infiniband/hw/cxgb3/cxio_hal.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c b/drivers/infiniband/hw/cxgb3/cxio_hal.c
index 8ac72ac7cbac..0e37f55678f8 100644
--- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
+++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
@@ -174,7 +174,6 @@ int cxio_create_cq(struct cxio_rdev *rdev_p, struct t3_cq *cq, int kernel)
return -ENOMEM;
}
dma_unmap_addr_set(cq, mapping, cq->dma_addr);
- memset(cq->queue, 0, size);
setup.id = cq->cqid;
setup.base_addr = (u64) (cq->dma_addr);
setup.size = 1UL << cq->size_log2;
@@ -538,8 +537,6 @@ static int cxio_hal_init_ctrl_qp(struct cxio_rdev *rdev_p)
dma_unmap_addr_set(&rdev_p->ctrl_qp, mapping,
rdev_p->ctrl_qp.dma_addr);
rdev_p->ctrl_qp.doorbell = (void __iomem *)rdev_p->rnic_info.kdb_addr;
- memset(rdev_p->ctrl_qp.workq, 0,
- (1 << T3_CTRL_QP_SIZE_LOG2) * sizeof(union t3_wr));
mutex_init(&rdev_p->ctrl_qp.lock);
init_waitqueue_head(&rdev_p->ctrl_qp.waitq);
--
2.11.0
^ permalink raw reply related
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-27 17:00 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Christoph Hellwig, Jason Gunthorpe, linux-kernel, linux-block,
linux-nvme, linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas,
Dan Williams, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <89889319-e778-7772-ab36-dc55b59826be@deltatee.com>
On Thu, Jun 27, 2019 at 10:30:42AM -0600, Logan Gunthorpe wrote:
> > (a) a range is normal RAM, DMA mapping works as usual
> > (b) a range is another devices BAR, in which case we need to do a
> > map_resource equivalent (which really just means don't bother with
> > cache flush on non-coherent architectures) and apply any needed
> > offset, fixed or iommu based
>
> Well I would split this into two cases: (b1) ranges in another device's
> BAR that will pass through the root complex and require a map_resource
> equivalent and (b2) ranges in another device's bar that don't pass
> through the root complex and require applying an offset to the bus
> address. Both require rather different handling and the submitting
> driver should already know ahead of time what type we have.
True.
>
> > (c) a range points to a BAR on the acting device. In which case we
> > don't need to DMA map at all, because no dma is happening but just an
> > internal transfer. And depending on the device that might also require
> > a different addressing mode
>
> I think (c) is actually just a special case of (b2). Any device that has
> a special protocol for addressing the local BAR can just do a range
> compare on the address to determine if it's local or not. Devices that
> don't have a special protocol for this would handle both (c) and (b2)
> the same.
It is not. (c) is fundamentally very different as it is not actually
an operation that ever goes out to the wire at all, and which is why the
actual physical address on the wire does not matter at all.
Some interfaces like NVMe have designed it in a way that it the commands
used to do this internal transfer look like (b2), but that is just their
(IMHO very questionable) interface design choice, that produces a whole
chain of problems.
> > I guess it might make sense to just have a block layer flag that (b) or
> > (c) might be contained in a bio. Then we always look up the data
> > structure, but can still fall back to (a) if nothing was found. That
> > even allows free mixing and matching of memory types, at least as long
> > as they are contained to separate bio_vec segments.
>
> IMO these three cases should be reflected in flags in the bio_vec. We'd
> probably still need a queue flag to indicate support for mapping these,
> but a flag on the bio that indicates special cases *might* exist in the
> bio_vec and the driver has to do extra work to somehow distinguish the
> three types doesn't seem useful. bio_vec flags also make it easy to
> support mixing segments from different memory types.
So I іnitially suggested these flags. But without a pgmap we absolutely
need a lookup operation to find which phys address ranges map to which
device. And once we do that the data structure the only thing we need
is a flag saying that we need that information, and everything else
can be in the data structure returned from that lookup.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-27 16:49 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190627163504.GB9568@ziepe.ca>
On 2019-06-27 10:35 a.m., Jason Gunthorpe wrote:
> On Thu, Jun 27, 2019 at 10:09:41AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2019-06-27 12:32 a.m., Jason Gunthorpe wrote:
>>> On Wed, Jun 26, 2019 at 03:18:07PM -0600, Logan Gunthorpe wrote:
>>>>> I don't think we should make drives do that. What if it got CMB memory
>>>>> on some other device?
>>>>
>>>> Huh? A driver submitting P2P requests finds appropriate memory to use
>>>> based on the DMA device that will be doing the mapping. It *has* to. It
>>>> doesn't necessarily have control over which P2P provider it might find
>>>> (ie. it may get CMB memory from a random NVMe device), but it easily
>>>> knows the NVMe device it got the CMB memory for. Look at the existing
>>>> code in the nvme target.
>>>
>>> No, this all thinking about things from the CMB perspective. With CMB
>>> you don't care about the BAR location because it is just a temporary
>>> buffer. That is a unique use model.
>>>
>>> Every other case has data residing in BAR memory that can really only
>>> reside in that one place (ie on a GPU/FPGA DRAM or something). When an IO
>>> against that is run it should succeed, even if that means bounce
>>> buffering the IO - as the user has really asked for this transfer to
>>> happen.
>>>
>>> We certainly don't get to generally pick where the data resides before
>>> starting the IO, that luxury is only for CMB.
>>
>> I disagree. If we we're going to implement a "bounce" we'd probably want
>> to do it in two DMA requests.
>
> How do you mean?
>
>> So the GPU/FPGA driver would first decide whether it can do it P2P
>> directly and, if it can't, would want to submit a DMA request copy
>> the data to host memory and then submit an IO normally to the data's
>> final destination.
>
> I don't think a GPU/FPGA driver will be involved, this would enter the
> block layer through the O_DIRECT path or something generic.. This the
> general flow I was suggesting to Dan earlier
I would say the O_DIRECT path has to somehow call into the driver
backing the VMA to get an address to appropriate memory (in some way
vaguely similar to how we were discussing at LSF/MM). If P2P can't be
done at that point, then the provider driver would do the copy to system
memory, in the most appropriate way, and return regular pages for
O_DIRECT to submit to the block device.
>> I think it would be a larger layering violation to have the NVMe driver
>> (for example) memcpy data off a GPU's bar during a dma_map step to
>> support this bouncing. And it's even crazier to expect a DMA transfer to
>> be setup in the map step.
>
> Why? Don't we already expect the DMA mapper to handle bouncing for
> lots of cases, how is this case different? This is the best place to
> place it to make it shared.
This is different because it's special memory where the DMA mapper can't
possibly know the best way to transfer the data. The best way to
transfer the data is almost certainly going to be a DMA request handled
by the GPU/FPGA. So, one way or another, the GPU/FPGA driver has to be
involved.
One could argue that the hook to the GPU/FPGA driver could be in the
mapping step but then we'd have to do lookups based on an address --
where as the VMA could more easily have a hook back to whatever driver
exported it.
Logan
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Jason Gunthorpe @ 2019-06-27 16:35 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <6afe4027-26c8-df4e-65ce-49df07dec54d@deltatee.com>
On Thu, Jun 27, 2019 at 10:09:41AM -0600, Logan Gunthorpe wrote:
>
>
> On 2019-06-27 12:32 a.m., Jason Gunthorpe wrote:
> > On Wed, Jun 26, 2019 at 03:18:07PM -0600, Logan Gunthorpe wrote:
> >>> I don't think we should make drives do that. What if it got CMB memory
> >>> on some other device?
> >>
> >> Huh? A driver submitting P2P requests finds appropriate memory to use
> >> based on the DMA device that will be doing the mapping. It *has* to. It
> >> doesn't necessarily have control over which P2P provider it might find
> >> (ie. it may get CMB memory from a random NVMe device), but it easily
> >> knows the NVMe device it got the CMB memory for. Look at the existing
> >> code in the nvme target.
> >
> > No, this all thinking about things from the CMB perspective. With CMB
> > you don't care about the BAR location because it is just a temporary
> > buffer. That is a unique use model.
> >
> > Every other case has data residing in BAR memory that can really only
> > reside in that one place (ie on a GPU/FPGA DRAM or something). When an IO
> > against that is run it should succeed, even if that means bounce
> > buffering the IO - as the user has really asked for this transfer to
> > happen.
> >
> > We certainly don't get to generally pick where the data resides before
> > starting the IO, that luxury is only for CMB.
>
> I disagree. If we we're going to implement a "bounce" we'd probably want
> to do it in two DMA requests.
How do you mean?
> So the GPU/FPGA driver would first decide whether it can do it P2P
> directly and, if it can't, would want to submit a DMA request copy
> the data to host memory and then submit an IO normally to the data's
> final destination.
I don't think a GPU/FPGA driver will be involved, this would enter the
block layer through the O_DIRECT path or something generic.. This the
general flow I was suggesting to Dan earlier
> I think it would be a larger layering violation to have the NVMe driver
> (for example) memcpy data off a GPU's bar during a dma_map step to
> support this bouncing. And it's even crazier to expect a DMA transfer to
> be setup in the map step.
Why? Don't we already expect the DMA mapper to handle bouncing for
lots of cases, how is this case different? This is the best place to
place it to make it shared.
Jason
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-27 16:30 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, linux-kernel, linux-block, linux-nvme, linux-pci,
linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190627090843.GB11548@lst.de>
On 2019-06-27 3:08 a.m., Christoph Hellwig wrote:
> On Wed, Jun 26, 2019 at 02:45:38PM -0600, Logan Gunthorpe wrote:
>>> The bar info would give the exporting struct device and any other info
>>> we need to make the iommu mapping.
>>
>> Well, the IOMMU mapping is the normal thing the mapping driver will
>> always do. We'd really just need the submitting driver to, when
>> appropriate, inform the mapping driver that this is a pci bus address
>> and not to call dma_map_xxx(). Then, for special mappings for the CMB
>> like Christoph is talking about, it's simply a matter of doing a range
>> compare on the PCI Bus address and converting the bus address to a BAR
>> and offset.
>
> Well, range compare on the physical address. We have a few different
> options here:
>
> (a) a range is normal RAM, DMA mapping works as usual
> (b) a range is another devices BAR, in which case we need to do a
> map_resource equivalent (which really just means don't bother with
> cache flush on non-coherent architectures) and apply any needed
> offset, fixed or iommu based
Well I would split this into two cases: (b1) ranges in another device's
BAR that will pass through the root complex and require a map_resource
equivalent and (b2) ranges in another device's bar that don't pass
through the root complex and require applying an offset to the bus
address. Both require rather different handling and the submitting
driver should already know ahead of time what type we have.
> (c) a range points to a BAR on the acting device. In which case we
> don't need to DMA map at all, because no dma is happening but just an
> internal transfer. And depending on the device that might also require
> a different addressing mode
I think (c) is actually just a special case of (b2). Any device that has
a special protocol for addressing the local BAR can just do a range
compare on the address to determine if it's local or not. Devices that
don't have a special protocol for this would handle both (c) and (b2)
the same.
> I guess it might make sense to just have a block layer flag that (b) or
> (c) might be contained in a bio. Then we always look up the data
> structure, but can still fall back to (a) if nothing was found. That
> even allows free mixing and matching of memory types, at least as long
> as they are contained to separate bio_vec segments.
IMO these three cases should be reflected in flags in the bio_vec. We'd
probably still need a queue flag to indicate support for mapping these,
but a flag on the bio that indicates special cases *might* exist in the
bio_vec and the driver has to do extra work to somehow distinguish the
three types doesn't seem useful. bio_vec flags also make it easy to
support mixing segments from different memory types.
Logan
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Logan Gunthorpe @ 2019-06-27 16:09 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <20190627063223.GA7736@ziepe.ca>
On 2019-06-27 12:32 a.m., Jason Gunthorpe wrote:
> On Wed, Jun 26, 2019 at 03:18:07PM -0600, Logan Gunthorpe wrote:
>>> I don't think we should make drives do that. What if it got CMB memory
>>> on some other device?
>>
>> Huh? A driver submitting P2P requests finds appropriate memory to use
>> based on the DMA device that will be doing the mapping. It *has* to. It
>> doesn't necessarily have control over which P2P provider it might find
>> (ie. it may get CMB memory from a random NVMe device), but it easily
>> knows the NVMe device it got the CMB memory for. Look at the existing
>> code in the nvme target.
>
> No, this all thinking about things from the CMB perspective. With CMB
> you don't care about the BAR location because it is just a temporary
> buffer. That is a unique use model.
>
> Every other case has data residing in BAR memory that can really only
> reside in that one place (ie on a GPU/FPGA DRAM or something). When an IO
> against that is run it should succeed, even if that means bounce
> buffering the IO - as the user has really asked for this transfer to
> happen.
>
> We certainly don't get to generally pick where the data resides before
> starting the IO, that luxury is only for CMB.
I disagree. If we we're going to implement a "bounce" we'd probably want
to do it in two DMA requests. So the GPU/FPGA driver would first decide
whether it can do it P2P directly and, if it can't, would want to submit
a DMA request copy the data to host memory and then submit an IO
normally to the data's final destination.
I think it would be a larger layering violation to have the NVMe driver
(for example) memcpy data off a GPU's bar during a dma_map step to
support this bouncing. And it's even crazier to expect a DMA transfer to
be setup in the map step.
Logan
^ permalink raw reply
* Re: [PATCH v4 hmm 12/12] mm/hmm: Fix error flows in hmm_invalidate_range_start
From: Jason Gunthorpe @ 2019-06-27 16:06 UTC (permalink / raw)
To: Ralph Campbell
Cc: Andrea Arcangeli, Philip Yang,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Jerome Glisse,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Ira Weiny, Christoph Hellwig, Ben Skeggs
In-Reply-To: <035fa354-6caa-3738-b84d-20804813009a-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Wed, Jun 26, 2019 at 11:18:23AM -0700, Ralph Campbell wrote:
> > diff --git a/mm/hmm.c b/mm/hmm.c
> > index b224ea635a7716..89549eac03d506 100644
> > +++ b/mm/hmm.c
> > @@ -64,7 +64,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> > init_rwsem(&hmm->mirrors_sem);
> > hmm->mmu_notifier.ops = NULL;
> > INIT_LIST_HEAD(&hmm->ranges);
> > - mutex_init(&hmm->lock);
> > + spin_lock_init(&hmm->ranges_lock);
> > kref_init(&hmm->kref);
> > hmm->notifiers = 0;
> > hmm->mm = mm;
> > @@ -144,6 +144,23 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> > hmm_put(hmm);
> > }
> > +static void notifiers_decrement(struct hmm *hmm)
> > +{
> > + lockdep_assert_held(&hmm->ranges_lock);
> > +
>
> Why not acquire the lock here and release at the end instead
> of asserting the lock is held?
> It looks like everywhere notifiers_decrement() is called does
> that.
Yes, this is just some left over mistake, thanks
From aa371c720a9e3c632dcd9a6a2c73e325b9b2b98c Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgg@mellanox.com>
Date: Fri, 7 Jun 2019 12:10:33 -0300
Subject: [PATCH] mm/hmm: Fix error flows in hmm_invalidate_range_start
If the trylock on the hmm->mirrors_sem fails the function will return
without decrementing the notifiers that were previously incremented. Since
the caller will not call invalidate_range_end() on EAGAIN this will result
in notifiers becoming permanently incremented and deadlock.
If the sync_cpu_device_pagetables() required blocking the function will
not return EAGAIN even though the device continues to touch the
pages. This is a violation of the mmu notifier contract.
Switch, and rename, the ranges_lock to a spin lock so we can reliably
obtain it without blocking during error unwind.
The error unwind is necessary since the notifiers count must be held
incremented across the call to sync_cpu_device_pagetables() as we cannot
allow the range to become marked valid by a parallel
invalidate_start/end() pair while doing sync_cpu_device_pagetables().
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Philip Yang <Philip.Yang@amd.com>
---
v4
- Move lock into notifiers_decrement() (Ralph)
---
include/linux/hmm.h | 2 +-
mm/hmm.c | 69 ++++++++++++++++++++++++++-------------------
2 files changed, 41 insertions(+), 30 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index bf013e96525771..0fa8ea34ccef6d 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -86,7 +86,7 @@
struct hmm {
struct mm_struct *mm;
struct kref kref;
- struct mutex lock;
+ spinlock_t ranges_lock;
struct list_head ranges;
struct list_head mirrors;
struct mmu_notifier mmu_notifier;
diff --git a/mm/hmm.c b/mm/hmm.c
index b224ea635a7716..de35289df20d43 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -64,7 +64,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
init_rwsem(&hmm->mirrors_sem);
hmm->mmu_notifier.ops = NULL;
INIT_LIST_HEAD(&hmm->ranges);
- mutex_init(&hmm->lock);
+ spin_lock_init(&hmm->ranges_lock);
kref_init(&hmm->kref);
hmm->notifiers = 0;
hmm->mm = mm;
@@ -144,6 +144,25 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
hmm_put(hmm);
}
+static void notifiers_decrement(struct hmm *hmm)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
+ hmm->notifiers--;
+ if (!hmm->notifiers) {
+ struct hmm_range *range;
+
+ list_for_each_entry(range, &hmm->ranges, list) {
+ if (range->valid)
+ continue;
+ range->valid = true;
+ }
+ wake_up_all(&hmm->wq);
+ }
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
+}
+
static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
{
@@ -151,6 +170,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
+ unsigned long flags;
int ret = 0;
if (!kref_get_unless_zero(&hmm->kref))
@@ -161,12 +181,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
update.event = HMM_UPDATE_INVALIDATE;
update.blockable = mmu_notifier_range_blockable(nrange);
- if (mmu_notifier_range_blockable(nrange))
- mutex_lock(&hmm->lock);
- else if (!mutex_trylock(&hmm->lock)) {
- ret = -EAGAIN;
- goto out;
- }
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, &hmm->ranges, list) {
if (update.end < range->start || update.start >= range->end)
@@ -174,7 +189,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
range->valid = false;
}
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
if (mmu_notifier_range_blockable(nrange))
down_read(&hmm->mirrors_sem);
@@ -182,16 +197,23 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
ret = -EAGAIN;
goto out;
}
+
list_for_each_entry(mirror, &hmm->mirrors, list) {
- int ret;
+ int rc;
- ret = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
- if (!update.blockable && ret == -EAGAIN)
+ rc = mirror->ops->sync_cpu_device_pagetables(mirror, &update);
+ if (rc) {
+ if (WARN_ON(update.blockable || rc != -EAGAIN))
+ continue;
+ ret = -EAGAIN;
break;
+ }
}
up_read(&hmm->mirrors_sem);
out:
+ if (ret)
+ notifiers_decrement(hmm);
hmm_put(hmm);
return ret;
}
@@ -204,20 +226,7 @@ static void hmm_invalidate_range_end(struct mmu_notifier *mn,
if (!kref_get_unless_zero(&hmm->kref))
return;
- mutex_lock(&hmm->lock);
- hmm->notifiers--;
- if (!hmm->notifiers) {
- struct hmm_range *range;
-
- list_for_each_entry(range, &hmm->ranges, list) {
- if (range->valid)
- continue;
- range->valid = true;
- }
- wake_up_all(&hmm->wq);
- }
- mutex_unlock(&hmm->lock);
-
+ notifiers_decrement(hmm);
hmm_put(hmm);
}
@@ -868,6 +877,7 @@ int hmm_range_register(struct hmm_range *range,
{
unsigned long mask = ((1UL << page_shift) - 1UL);
struct hmm *hmm = mirror->hmm;
+ unsigned long flags;
range->valid = false;
range->hmm = NULL;
@@ -886,7 +896,7 @@ int hmm_range_register(struct hmm_range *range,
return -EFAULT;
/* Initialize range to track CPU page table updates. */
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
range->hmm = hmm;
kref_get(&hmm->kref);
@@ -898,7 +908,7 @@ int hmm_range_register(struct hmm_range *range,
*/
if (!hmm->notifiers)
range->valid = true;
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
return 0;
}
@@ -914,10 +924,11 @@ EXPORT_SYMBOL(hmm_range_register);
void hmm_range_unregister(struct hmm_range *range)
{
struct hmm *hmm = range->hmm;
+ unsigned long flags;
- mutex_lock(&hmm->lock);
+ spin_lock_irqsave(&hmm->ranges_lock, flags);
list_del_init(&range->list);
- mutex_unlock(&hmm->lock);
+ spin_unlock_irqrestore(&hmm->ranges_lock, flags);
/* Drop reference taken by hmm_range_register() */
mmput(hmm->mm);
--
2.22.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-27 9:08 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Jason Gunthorpe, Christoph Hellwig, linux-kernel, linux-block,
linux-nvme, linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas,
Dan Williams, Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <8a0a08c3-a537-bff6-0852-a5f337a70688@deltatee.com>
On Wed, Jun 26, 2019 at 02:45:38PM -0600, Logan Gunthorpe wrote:
> > The bar info would give the exporting struct device and any other info
> > we need to make the iommu mapping.
>
> Well, the IOMMU mapping is the normal thing the mapping driver will
> always do. We'd really just need the submitting driver to, when
> appropriate, inform the mapping driver that this is a pci bus address
> and not to call dma_map_xxx(). Then, for special mappings for the CMB
> like Christoph is talking about, it's simply a matter of doing a range
> compare on the PCI Bus address and converting the bus address to a BAR
> and offset.
Well, range compare on the physical address. We have a few different
options here:
(a) a range is normal RAM, DMA mapping works as usual
(b) a range is another devices BAR, in which case we need to do a
map_resource equivalent (which really just means don't bother with
cache flush on non-coherent architectures) and apply any needed
offset, fixed or iommu based
(c) a range points to a BAR on the acting device. In which case we
don't need to DMA map at all, because no dma is happening but just an
internal transfer. And depending on the device that might also require
a different addressing mode
I guess it might make sense to just have a block layer flag that (b) or
(c) might be contained in a bio. Then we always look up the data
structure, but can still fall back to (a) if nothing was found. That
even allows free mixing and matching of memory types, at least as long
as they are contained to separate bio_vec segments.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Christoph Hellwig @ 2019-06-27 9:01 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Jason Gunthorpe, Stephen Bates
In-Reply-To: <c15d5997-9ba4-f7db-0e7a-a69e75df316c@deltatee.com>
On Wed, Jun 26, 2019 at 12:31:08PM -0600, Logan Gunthorpe wrote:
> > we have a hole behind len where we could store flag. Preferably
> > optionally based on a P2P or other magic memory types config
> > option so that 32-bit systems with 32-bit phys_addr_t actually
> > benefit from the smaller and better packing structure.
>
> That seems sensible. The one thing that's unclear though is how to get
> the PCI Bus address when appropriate. Can we pass that in instead of the
> phys_addr with an appropriate flag? Or will we need to pass the actual
> physical address and then, at the map step, the driver has to some how
> lookup the PCI device to figure out the bus offset?
Yes, I think we'll need a lookup mechanism of some kind.
^ permalink raw reply
* Re: [RFC PATCH 00/28] Removing struct page from P2PDMA
From: Jason Gunthorpe @ 2019-06-27 6:32 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Christoph Hellwig, linux-kernel, linux-block, linux-nvme,
linux-pci, linux-rdma, Jens Axboe, Bjorn Helgaas, Dan Williams,
Sagi Grimberg, Keith Busch, Stephen Bates
In-Reply-To: <c25d3333-dcd5-3313-089b-7fbbd6fbd876@deltatee.com>
On Wed, Jun 26, 2019 at 03:18:07PM -0600, Logan Gunthorpe wrote:
> > I don't think we should make drives do that. What if it got CMB memory
> > on some other device?
>
> Huh? A driver submitting P2P requests finds appropriate memory to use
> based on the DMA device that will be doing the mapping. It *has* to. It
> doesn't necessarily have control over which P2P provider it might find
> (ie. it may get CMB memory from a random NVMe device), but it easily
> knows the NVMe device it got the CMB memory for. Look at the existing
> code in the nvme target.
No, this all thinking about things from the CMB perspective. With CMB
you don't care about the BAR location because it is just a temporary
buffer. That is a unique use model.
Every other case has data residing in BAR memory that can really only
reside in that one place (ie on a GPU/FPGA DRAM or something). When an IO
against that is run it should succeed, even if that means bounce
buffering the IO - as the user has really asked for this transfer to
happen.
We certainly don't get to generally pick where the data resides before
starting the IO, that luxury is only for CMB.
> > I think with some simple caching this will become negligible for cases
> > you care about
>
> Well *maybe* it will be negligible performance wise, but it's also a lot
> more complicated, code wise. Tree lookups will always be a lot more
> expensive than just checking a flag.
Interval trees are pretty simple API wise, and if we only populate
them with P2P providers you probably find the tree depth is negligible
in current systems with one or two P2P providers.
Jason
^ permalink raw reply
* Re: [for-next V2 09/10] RDMA/nldev: Added configuration of RDMA dynamic interrupt moderation to netlink
From: Yamin Friedman @ 2019-06-27 5:29 UTC (permalink / raw)
To: Sagi Grimberg, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <f621af3b-37a3-eb97-368a-3201fa49f338@grimberg.me>
On 6/26/2019 12:15 AM, Sagi Grimberg wrote:
>
>
> On 6/25/19 1:57 PM, Saeed Mahameed wrote:
>> From: Yamin Friedman <yaminf@mellanox.com>
>>
>> Added parameter in ib_device for enabling dynamic interrupt
>> moderation so
>> that it can be configured in userspace using rdma tool.
>>
>> In order to set dim for an ib device the command is:
>> rdma dev set [DEV] dim [on|off]
>> Please set on/off.
>
> Is "dim" what you want to expose to the user? maybe
> "adaptive-moderation" is more friendly?
That makes sense, I will change it.
^ permalink raw reply
* Re: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
From: Yamin Friedman @ 2019-06-27 5:28 UTC (permalink / raw)
To: Sagi Grimberg, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Max Gurtovoy
In-Reply-To: <adb3687a-6db3-b1a4-cd32-8b4889550c81@grimberg.me>
On 6/26/2019 12:14 AM, Sagi Grimberg wrote:
>
>
>> +static int ib_poll_dim_handler(struct irq_poll *iop, int budget)
>> +{
>> + struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
>> + struct dim *dim = cq->dim;
>> + int completed;
>> +
>> + completed = __ib_process_cq(cq, budget, cq->wc, IB_POLL_BATCH);
>> + if (completed < budget) {
>> + irq_poll_complete(&cq->iop);
>> + if (ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
>> + irq_poll_sched(&cq->iop);
>> + }
>> +
>> + rdma_dim(dim, completed);
>
> Why duplicate the entire thing for a one-liner?
You are right, this was leftover from a previous version where there
were more significant changes. I will remove the extra function.
>
>> +
>> + return completed;
>> +}
>> +
>> static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
>> {
>> irq_poll_sched(&cq->iop);
>> @@ -105,14 +157,18 @@ static void ib_cq_completion_softirq(struct
>> ib_cq *cq, void *private)
>> static void ib_cq_poll_work(struct work_struct *work)
>> {
>> - struct ib_cq *cq = container_of(work, struct ib_cq, work);
>> + struct ib_cq *cq = container_of(work, struct ib_cq,
>> + work);
>
> Why was that changed?
I will fix this.
>
>> int completed;
>> completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE,
>> cq->wc,
>> IB_POLL_BATCH);
>> +
>
> newline?
Same as above.
>
>> if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
>> ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
>> queue_work(cq->comp_wq, &cq->work);
>> + else if (cq->dim)
>> + rdma_dim(cq->dim, completed);
>> }
>> static void ib_cq_completion_workqueue(struct ib_cq *cq, void
>> *private)
>> @@ -166,6 +222,8 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device
>> *dev, void *private,
>> rdma_restrack_set_task(&cq->res, caller);
>> rdma_restrack_kadd(&cq->res);
>> + rdma_dim_init(cq);
>> +
>> switch (cq->poll_ctx) {
>> case IB_POLL_DIRECT:
>> cq->comp_handler = ib_cq_completion_direct;
>> @@ -173,7 +231,13 @@ struct ib_cq *__ib_alloc_cq_user(struct
>> ib_device *dev, void *private,
>> case IB_POLL_SOFTIRQ:
>> cq->comp_handler = ib_cq_completion_softirq;
>> - irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
>> + if (cq->dim) {
>> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
>> + ib_poll_dim_handler);
>> + } else
>> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
>> + ib_poll_handler);
>> +
>> ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
>> break;
>> case IB_POLL_WORKQUEUE:
>> @@ -226,6 +290,9 @@ void ib_free_cq_user(struct ib_cq *cq, struct
>> ib_udata *udata)
>> WARN_ON_ONCE(1);
>> }
>> + if (cq->dim)
>> + cancel_work_sync(&cq->dim->work);
>> + kfree(cq->dim);
>> kfree(cq->wc);
>> rdma_restrack_del(&cq->res);
>> ret = cq->device->ops.destroy_cq(cq, udata);
>> diff --git a/drivers/infiniband/hw/mlx5/main.c
>> b/drivers/infiniband/hw/mlx5/main.c
>> index abac70ad5c7c..b1b45dbe24a5 100644
>> --- a/drivers/infiniband/hw/mlx5/main.c
>> +++ b/drivers/infiniband/hw/mlx5/main.c
>> @@ -6305,6 +6305,8 @@ static int mlx5_ib_stage_caps_init(struct
>> mlx5_ib_dev *dev)
>> MLX5_CAP_GEN(dev->mdev, disable_local_lb_mc)))
>> mutex_init(&dev->lb.mutex);
>> + dev->ib_dev.use_cq_dim = true;
>> +
>
> Please don't. This is a bad choice to opt it in by default.
^ permalink raw reply
* Re: [for-next V2 08/10] linux/dim: Implement rdma_dim
From: Yamin Friedman @ 2019-06-27 5:25 UTC (permalink / raw)
To: Sagi Grimberg, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Max Gurtovoy
In-Reply-To: <bfa2159e-1576-6b3c-c85b-ee98bd4f9a47@grimberg.me>
On 6/26/2019 1:02 AM, Sagi Grimberg wrote:
>
>> +void rdma_dim(struct dim *dim, u64 completions)
>> +{
>> + struct dim_sample *curr_sample = &dim->measuring_sample;
>> + struct dim_stats curr_stats;
>> + u32 nevents;
>> +
>> + dim_update_sample_with_comps(curr_sample->event_ctr + 1,
>> + curr_sample->pkt_ctr,
>> + curr_sample->byte_ctr,
>> + curr_sample->comp_ctr + completions,
>> + &dim->measuring_sample);
>
> If this is the only caller, why add pkt_ctr and byte_ctr at all?
We wanted to keep the API general enough that if someone wants to
implement a different algorithm using the dim library they will be able
to use all the possible statistics. I agree though that in the rdma_dim
function there is no point in making it seem like they are relevant
parameters.
^ permalink raw reply
* [PATCH AUTOSEL 5.1 82/95] IB/hfi1: Handle port down properly in pio
From: Sasha Levin @ 2019-06-27 0:30 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Dennis Dalessandro, Doug Ledford, Sasha Levin,
linux-rdma
In-Reply-To: <20190627003021.19867-1-sashal@kernel.org>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
[ Upstream commit 942a899335707fc9cfc97cb382a60734b2ff4e03 ]
The call to sc_buffer_alloc currently returns NULL (no buffer) or
a buffer descriptor.
There is a third case when the port is down. Currently that
returns NULL and this prevents the caller from properly handling the
sc_buffer_alloc() failure. A verbs code link test after the call is
racy so the indication needs to come from the state check inside the allocation
routine to be valid.
Fix by encoding the ECOMM failure like SDMA. IS_ERR_OR_NULL() tests
are added at all call sites. For verbs send, this needs to treat any
error by returning a completion without any MMIO copy.
Fixes: 7724105686e7 ("IB/hfi1: add driver files")
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/hw/hfi1/pio.c | 5 +++--
drivers/infiniband/hw/hfi1/rc.c | 2 +-
drivers/infiniband/hw/hfi1/ud.c | 4 ++--
drivers/infiniband/hw/hfi1/verbs.c | 4 ++--
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
index 1ee47838d4de..17ea224fbecb 100644
--- a/drivers/infiniband/hw/hfi1/pio.c
+++ b/drivers/infiniband/hw/hfi1/pio.c
@@ -1443,7 +1443,8 @@ void sc_stop(struct send_context *sc, int flag)
* @cb: optional callback to call when the buffer is finished sending
* @arg: argument for cb
*
- * Return a pointer to a PIO buffer if successful, NULL if not enough room.
+ * Return a pointer to a PIO buffer, NULL if not enough room, -ECOMM
+ * when link is down.
*/
struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
pio_release_cb cb, void *arg)
@@ -1459,7 +1460,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
spin_lock_irqsave(&sc->alloc_lock, flags);
if (!(sc->flags & SCF_ENABLED)) {
spin_unlock_irqrestore(&sc->alloc_lock, flags);
- goto done;
+ return ERR_PTR(-ECOMM);
}
retry:
diff --git a/drivers/infiniband/hw/hfi1/rc.c b/drivers/infiniband/hw/hfi1/rc.c
index 24cbac277bf0..b7b74222eaf0 100644
--- a/drivers/infiniband/hw/hfi1/rc.c
+++ b/drivers/infiniband/hw/hfi1/rc.c
@@ -1434,7 +1434,7 @@ void hfi1_send_rc_ack(struct hfi1_packet *packet, bool is_fecn)
pbc = create_pbc(ppd, pbc_flags, qp->srate_mbps,
sc_to_vlt(ppd->dd, sc5), plen);
pbuf = sc_buffer_alloc(rcd->sc, plen, NULL, NULL);
- if (!pbuf) {
+ if (IS_ERR_OR_NULL(pbuf)) {
/*
* We have no room to send at the moment. Pass
* responsibility for sending the ACK to the send engine
diff --git a/drivers/infiniband/hw/hfi1/ud.c b/drivers/infiniband/hw/hfi1/ud.c
index f88ad425664a..4cb0fce5c096 100644
--- a/drivers/infiniband/hw/hfi1/ud.c
+++ b/drivers/infiniband/hw/hfi1/ud.c
@@ -683,7 +683,7 @@ void return_cnp_16B(struct hfi1_ibport *ibp, struct rvt_qp *qp,
pbc = create_pbc(ppd, pbc_flags, qp->srate_mbps, vl, plen);
if (ctxt) {
pbuf = sc_buffer_alloc(ctxt, plen, NULL, NULL);
- if (pbuf) {
+ if (!IS_ERR_OR_NULL(pbuf)) {
trace_pio_output_ibhdr(ppd->dd, &hdr, sc5);
ppd->dd->pio_inline_send(ppd->dd, pbuf, pbc,
&hdr, hwords);
@@ -738,7 +738,7 @@ void return_cnp(struct hfi1_ibport *ibp, struct rvt_qp *qp, u32 remote_qpn,
pbc = create_pbc(ppd, pbc_flags, qp->srate_mbps, vl, plen);
if (ctxt) {
pbuf = sc_buffer_alloc(ctxt, plen, NULL, NULL);
- if (pbuf) {
+ if (!IS_ERR_OR_NULL(pbuf)) {
trace_pio_output_ibhdr(ppd->dd, &hdr, sc5);
ppd->dd->pio_inline_send(ppd->dd, pbuf, pbc,
&hdr, hwords);
diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 8d64972c6226..ef290f1fdf63 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1039,10 +1039,10 @@ int hfi1_verbs_send_pio(struct rvt_qp *qp, struct hfi1_pkt_state *ps,
if (cb)
iowait_pio_inc(&priv->s_iowait);
pbuf = sc_buffer_alloc(sc, plen, cb, qp);
- if (unlikely(!pbuf)) {
+ if (unlikely(IS_ERR_OR_NULL(pbuf))) {
if (cb)
verbs_pio_complete(qp, 0);
- if (ppd->host_link_state != HLS_UP_ACTIVE) {
+ if (IS_ERR(pbuf)) {
/*
* If we have filled the PIO buffers to capacity and are
* not in an active state this request is not going to
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.1 81/95] IB/hfi1: Handle wakeup of orphaned QPs for pio
From: Sasha Levin @ 2019-06-27 0:30 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Dennis Dalessandro, Doug Ledford, Sasha Levin,
linux-rdma
In-Reply-To: <20190627003021.19867-1-sashal@kernel.org>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
[ Upstream commit 099a884ba4c00145cef283d36e050726311c2e95 ]
Once a send context is taken down due to a link failure, any QPs waiting
for pio credits will stay on the waitlist indefinitely.
Fix by wakeing up all QPs linked to piowait list.
Fixes: 7724105686e7 ("IB/hfi1: add driver files")
Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/hw/hfi1/pio.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
index a1de566fe95e..1ee47838d4de 100644
--- a/drivers/infiniband/hw/hfi1/pio.c
+++ b/drivers/infiniband/hw/hfi1/pio.c
@@ -952,6 +952,22 @@ void sc_disable(struct send_context *sc)
}
}
spin_unlock(&sc->release_lock);
+
+ write_seqlock(&sc->waitlock);
+ while (!list_empty(&sc->piowait)) {
+ struct iowait *wait;
+ struct rvt_qp *qp;
+ struct hfi1_qp_priv *priv;
+
+ wait = list_first_entry(&sc->piowait, struct iowait, list);
+ qp = iowait_to_qp(wait);
+ priv = qp->priv;
+ list_del_init(&priv->s_iowait.list);
+ priv->s_iowait.lock = NULL;
+ hfi1_qp_wakeup(qp, RVT_S_WAIT_PIO | HFI1_S_WAIT_PIO_DRAIN);
+ }
+ write_sequnlock(&sc->waitlock);
+
spin_unlock_irq(&sc->alloc_lock);
}
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.1 80/95] IB/hfi1: Wakeup QPs orphaned on wait list after flush
From: Sasha Levin @ 2019-06-27 0:30 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Mike Marciniszyn, Kaike Wan, Dennis Dalessandro, Doug Ledford,
Sasha Levin, linux-rdma
In-Reply-To: <20190627003021.19867-1-sashal@kernel.org>
From: Mike Marciniszyn <mike.marciniszyn@intel.com>
[ Upstream commit f972775b1cc0441ae22c9f8d06dd16b118463632 ]
Once an SDMA engine is taken down due to a link failure, any waiting QPs
that do not have outstanding descriptors in the ring will stay
on the dmawait list as long as the port is down.
Since there is no timer running, they will stay there for a long time.
The fix is to wake up all iowaits linked to dmawait. The send engine
will build and post packets that get flushed back.
Fixes: 7724105686e7 ("IB/hfi1: add driver files")
Reviewed-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/infiniband/hw/hfi1/sdma.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/infiniband/hw/hfi1/sdma.c b/drivers/infiniband/hw/hfi1/sdma.c
index b0110728f541..1cde1b8f0c8b 100644
--- a/drivers/infiniband/hw/hfi1/sdma.c
+++ b/drivers/infiniband/hw/hfi1/sdma.c
@@ -405,6 +405,7 @@ static void sdma_flush(struct sdma_engine *sde)
struct sdma_txreq *txp, *txp_next;
LIST_HEAD(flushlist);
unsigned long flags;
+ uint seq;
/* flush from head to tail */
sdma_flush_descq(sde);
@@ -418,6 +419,22 @@ static void sdma_flush(struct sdma_engine *sde)
/* flush from flush list */
list_for_each_entry_safe(txp, txp_next, &flushlist, list)
complete_tx(sde, txp, SDMA_TXREQ_S_ABORTED);
+ /* wakeup QPs orphaned on the dmawait list */
+ do {
+ struct iowait *w, *nw;
+
+ seq = read_seqbegin(&sde->waitlock);
+ if (!list_empty(&sde->dmawait)) {
+ write_seqlock(&sde->waitlock);
+ list_for_each_entry_safe(w, nw, &sde->dmawait, list) {
+ if (w->wakeup) {
+ w->wakeup(w, SDMA_AVAIL_REASON);
+ list_del_init(&w->list);
+ }
+ }
+ write_sequnlock(&sde->waitlock);
+ }
+ } while (read_seqretry(&sde->waitlock, seq));
}
/*
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox