Re: [RFC net-next 5/7] net/ism: Move ism_loopback to net/ism

Dust Li
2025-01-20T03:55:35Z

On 2025-01-15 20:55:25, Alexandra Winter wrote:
>The first stage of ism_loopback was implemented as part of the
>SMC module [1]. Now that we have the ism layer, provide
>access to the ism_loopback device to all ism clients.
>
>Move ism_loopback.* from net/smc to net/ism.
>The following changes are required to ism_loopback.c:
>- Change ism_lo_move_data() to no longer schedule an smcd receive tasklet,
>but instead call ism_client->handle_irq().
>Note: In this RFC patch ism_loppback is not fully generic.
> The smc-d client uses attached buffers, for moves without signalling.
> and not-attached buffers for moves with signalling.
> ism_lo_move_data() must not rely on that assumption.
> ism_lo_move_data() must be able to handle more than one ism client.
>
>In addition the following changes are required to unify ism_loopback and
>ism_vp:
>
>In ism layer and ism_vpci:
>ism_loopback is not backed by a pci device, so use dev instead of pdev in
>ism_dev.
>
>In smc-d:
>in smcd_alloc_dev():
>- use kernel memory instead of device memory for smcd_dev and smcd->conn.
> An alternative would be to ask device to alloc the memory.
>- use different smcd_ops and max_dmbs for ism_vp and ism_loopback.
> A future patch can change smc-d to directly use ism_ops instead of
> smcd_ops.
>- use ism dev_name instead of pci dev name for ism_evt_wq name
>- allocate an event workqueue for ism_loopback, although it currently does
> not generate events.
>
>Link: https://lore.kernel.org/linux-kernel//20240428060738.60843-1-guwen@linux.alibaba.com/ [1]
>
>Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
>---
> drivers/s390/net/ism.h | 6 +-
> drivers/s390/net/ism_drv.c | 31 ++-
> include/linux/ism.h | 59 +++++
> include/net/smc.h | 4 +-
> net/ism/Kconfig | 13 ++
> net/ism/Makefile | 1 +
> net/ism/ism_loopback.c | 366 +++++++++++++++++++++++++++++++
> net/ism/ism_loopback.h | 59 +++++
> net/ism/ism_main.c | 11 +-
> net/smc/Kconfig | 13 --
> net/smc/Makefile | 1 -
> net/smc/af_smc.c | 12 +-
> net/smc/smc_ism.c | 108 +++++++---
> net/smc/smc_loopback.c | 427 -------------------------------------
> net/smc/smc_loopback.h | 60 ------
> 15 files changed, 606 insertions(+), 565 deletions(-)
> create mode 100644 net/ism/ism_loopback.c
> create mode 100644 net/ism/ism_loopback.h
> delete mode 100644 net/smc/smc_loopback.c
> delete mode 100644 net/smc/smc_loopback.h
>
>diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
>index 61cf10334170..0deca6d0e328 100644
>--- a/drivers/s390/net/ism.h
>+++ b/drivers/s390/net/ism.h
>@@ -202,7 +202,7 @@ struct ism_sba {
> static inline void __ism_read_cmd(struct ism_dev *ism, void *data,
> unsigned long offset, unsigned long len)
> {
>- struct zpci_dev *zdev = to_zpci(ism->pdev);
>+ struct zpci_dev *zdev = to_zpci(to_pci_dev(ism->dev.parent));
> u64 req = ZPCI_CREATE_REQ(zdev->fh, 2, 8);
>
> while (len > 0) {
>@@ -216,7 +216,7 @@ static inline void __ism_read_cmd(struct ism_dev *ism, void *data,
> static inline void __ism_write_cmd(struct ism_dev *ism, void *data,
> unsigned long offset, unsigned long len)
> {
>- struct zpci_dev *zdev = to_zpci(ism->pdev);
>+ struct zpci_dev *zdev = to_zpci(to_pci_dev(ism->dev.parent));
> u64 req = ZPCI_CREATE_REQ(zdev->fh, 2, len);
>
> if (len)
>@@ -226,7 +226,7 @@ static inline void __ism_write_cmd(struct ism_dev *ism, void *data,
> static inline int __ism_move(struct ism_dev *ism, u64 dmb_req, void *data,
> unsigned int size)
> {
>- struct zpci_dev *zdev = to_zpci(ism->pdev);
>+ struct zpci_dev *zdev = to_zpci(to_pci_dev(ism->dev.parent));
> u64 req = ZPCI_CREATE_REQ(zdev->fh, 0, size);
>
> return __zpci_store_block(data, req, dmb_req);
>diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
>index ab70debbdd9d..c0954d6dd9f5 100644
>--- a/drivers/s390/net/ism_drv.c
>+++ b/drivers/s390/net/ism_drv.c
>@@ -88,7 +88,7 @@ static int register_sba(struct ism_dev *ism)
> dma_addr_t dma_handle;
> struct ism_sba *sba;
>
>- sba = dma_alloc_coherent(&ism->pdev->dev, PAGE_SIZE, &dma_handle,
>+ sba = dma_alloc_coherent(ism->dev.parent, PAGE_SIZE, &dma_handle,
> GFP_KERNEL);
> if (!sba)
> return -ENOMEM;
>@@ -99,7 +99,7 @@ static int register_sba(struct ism_dev *ism)
> cmd.request.sba = dma_handle;
>
> if (ism_cmd(ism, &cmd)) {
>- dma_free_coherent(&ism->pdev->dev, PAGE_SIZE, sba, dma_handle);
>+ dma_free_coherent(ism->dev.parent, PAGE_SIZE, sba, dma_handle);
> return -EIO;
> }
>
>@@ -115,7 +115,7 @@ static int register_ieq(struct ism_dev *ism)
> dma_addr_t dma_handle;
> struct ism_eq *ieq;
>
>- ieq = dma_alloc_coherent(&ism->pdev->dev, PAGE_SIZE, &dma_handle,
>+ ieq = dma_alloc_coherent(ism->dev.parent, PAGE_SIZE, &dma_handle,
> GFP_KERNEL);
> if (!ieq)
> return -ENOMEM;
>@@ -127,7 +127,7 @@ static int register_ieq(struct ism_dev *ism)
> cmd.request.len = sizeof(*ieq);
>
> if (ism_cmd(ism, &cmd)) {
>- dma_free_coherent(&ism->pdev->dev, PAGE_SIZE, ieq, dma_handle);
>+ dma_free_coherent(ism->dev.parent, PAGE_SIZE, ieq, dma_handle);
> return -EIO;
> }
>
>@@ -149,7 +149,7 @@ static int unregister_sba(struct ism_dev *ism)
> if (ret && ret != ISM_ERROR)
> return -EIO;
>
>- dma_free_coherent(&ism->pdev->dev, PAGE_SIZE,
>+ dma_free_coherent(ism->dev.parent, PAGE_SIZE,
> ism->sba, ism->sba_dma_addr);
>
> ism->sba = NULL;
>@@ -169,7 +169,7 @@ static int unregister_ieq(struct ism_dev *ism)
> if (ret && ret != ISM_ERROR)
> return -EIO;
>
>- dma_free_coherent(&ism->pdev->dev, PAGE_SIZE,
>+ dma_free_coherent(ism->dev.parent, PAGE_SIZE,
> ism->ieq, ism->ieq_dma_addr);
>
> ism->ieq = NULL;
>@@ -216,7 +216,7 @@ static int ism_query_rgid(struct ism_dev *ism, uuid_t *rgid, u32 vid_valid,
> static void ism_free_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
> {
> clear_bit(dmb->sba_idx, ism->sba_bitmap);
>- dma_unmap_page(&ism->pdev->dev, dmb->dma_addr, dmb->dmb_len,
>+ dma_unmap_page(ism->dev.parent, dmb->dma_addr, dmb->dmb_len,
> DMA_FROM_DEVICE);
> folio_put(virt_to_folio(dmb->cpu_addr));
> }
>@@ -227,7 +227,7 @@ static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
> unsigned long bit;
> int rc;
>
>- if (PAGE_ALIGN(dmb->dmb_len) > dma_get_max_seg_size(&ism->pdev->dev))
>+ if (PAGE_ALIGN(dmb->dmb_len) > dma_get_max_seg_size(ism->dev.parent))
> return -EINVAL;
>
> if (!dmb->sba_idx) {
>@@ -251,10 +251,10 @@ static int ism_alloc_dmb(struct ism_dev *ism, struct ism_dmb *dmb)
> }
>
> dmb->cpu_addr = folio_address(folio);
>- dmb->dma_addr = dma_map_page(&ism->pdev->dev,
>+ dmb->dma_addr = dma_map_page(ism->dev.parent,
> virt_to_page(dmb->cpu_addr), 0,
> dmb->dmb_len, DMA_FROM_DEVICE);
>- if (dma_mapping_error(&ism->pdev->dev, dmb->dma_addr)) {
>+ if (dma_mapping_error(ism->dev.parent, dmb->dma_addr)) {
> rc = -ENOMEM;
> goto out_free;
> }
>@@ -419,10 +419,7 @@ static int ism_supports_v2(void)
>
> static u16 ism_get_chid(struct ism_dev *ism)
> {
>- if (!ism || !ism->pdev)
>- return 0;
>-
>- return to_zpci(ism->pdev)->pchid;
>+ return to_zpci(to_pci_dev(ism->dev.parent))->pchid;
> }
>
> static void ism_handle_event(struct ism_dev *ism)
>@@ -499,7 +496,7 @@ static const struct ism_ops ism_vp_ops = {
>
> static int ism_dev_init(struct ism_dev *ism)
> {
>- struct pci_dev *pdev = ism->pdev;
>+ struct pci_dev *pdev = to_pci_dev(ism->dev.parent);
> int ret;
>
> ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
>@@ -565,7 +562,6 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
> spin_lock_init(&ism->lock);
> dev_set_drvdata(&pdev->dev, ism);
>- ism->pdev = pdev;
> ism->dev.parent = &pdev->dev;
> device_initialize(&ism->dev);
> dev_set_name(&ism->dev, dev_name(&pdev->dev));
>@@ -603,14 +599,13 @@ static int ism_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> device_del(&ism->dev);
> err_dev:
> dev_set_drvdata(&pdev->dev, NULL);
>- kfree(ism);
>
> return ret;
> }
>
> static void ism_dev_exit(struct ism_dev *ism)
> {
>- struct pci_dev *pdev = ism->pdev;
>+ struct pci_dev *pdev = to_pci_dev(ism->dev.parent);
> unsigned long flags;
> int i;
>
>diff --git a/include/linux/ism.h b/include/linux/ism.h
>index bc165d077071..929a1f275419 100644
>--- a/include/linux/ism.h
>+++ b/include/linux/ism.h
>@@ -144,6 +144,9 @@ int ism_unregister_client(struct ism_client *client);
> * identified by dmb_tok and idx. If signal flag (sf) then signal
> * the remote peer that data has arrived in this dmb.
> *
>+ * int (*unregister_dmb)(struct ism_dev *dev, struct ism_dmb *dmb);
>+ * Unregister an ism_dmb buffer
>+ *
> * int (*supports_v2)(void);
> *
> * u16 (*get_chid)(struct ism_dev *dev);
>@@ -218,12 +221,63 @@ struct ism_ops {
> int (*reset_vlan_required)(struct ism_dev *dev);
> int (*signal_event)(struct ism_dev *dev, uuid_t *rgid,
> u32 trigger_irq, u32 event_code, u64 info);
>+/* no copy option
>+ * --------------
>+ */
>+ /**
>+ * support_dmb_nocopy() - does this device provide no-copy option?
>+ * @dev: ism device
>+ *
>+ * In addition to using move_data(), a sender device can provide a
>+ * kernel address + length, that represents a target dmb
>+ * (like MMIO). If a sender writes into such a ghost-send-buffer
>+ * (= at this kernel address) the data will automatically
>+ * immediately appear in the target dmb, even without calling
>+ * move_data().
>+ * Note that this is NOT related to the MSG_ZEROCOPY socket flag.
>+ *
>+ * Either all 3 function pointers for support_dmb_nocopy(),
>+ * attach_dmb() and detach_dmb() are defined, or all of them must
>+ * be NULL.
>+ *
>+ * Return: non-zero, if no-copy is supported.
>+ */
>+ int (*support_dmb_nocopy)(struct ism_dev *dev);
>+ /**
>+ * attach_dmb() - attach local memory to a remote dmb
>+ * @dev: Local sending ism device
>+ * @dmb: all other parameters are passed in the form of a
>+ * dmb struct
>+ * TODO: (THIS IS CONFUSING, should be changed)

Agree.

>+ * dmb_tok: (in) Token of the remote dmb, we want to attach to
>+ * cpu_addr: (out) MMIO address
>+ * dma_addr: (out) MMIO address (if applicable, invalid otherwise)
>+ * dmb_len: (out) length of local MMIO region,
>+ * equal to length of remote DMB.
>+ * sba_idx: (out) index of remote dmb (NOT HELPFUL, should be removed)
>+ *
>+ * Provides a memory address to the sender that can be used to
>+ * directly write into the remote dmb.
>+ *
>+ * Return: Zero upon success, Error code otherwise
>+ */
>+ int (*attach_dmb)(struct ism_dev *dev, struct ism_dmb *dmb);
>+ /**
>+ * detach_dmb() - Detach the ghost buffer from a remote dmb
>+ * @dev: ism device
>+ * @token: dmb token of the remote dmb
>+ * Return: Zero upon success, Error code otherwise
>+ */
>+ int (*detach_dmb)(struct ism_dev *dev, u64 token);
> };
>

...

>+
>+static int ism_lo_move_data(struct ism_dev *ism, u64 dmb_tok,
>+ unsigned int idx, bool sf, unsigned int offset,
>+ void *data, unsigned int size)
>+{
>+ struct ism_lo_dmb_node *rmb_node = NULL, *tmp_node;
>+ struct ism_lo_dev *ldev;
>+ u16 s_mask;
>+ u8 client_id;
>+ u32 sba_idx;
>+
>+ ldev = container_of(ism, struct ism_lo_dev, ism);
>+
>+ if (!sf)
>+ /* since sndbuf is merged with peer DMB, there is
>+ * no need to copy data from sndbuf to peer DMB.
>+ */
>+ return 0;
>+
>+ read_lock_bh(&ldev->dmb_ht_lock);
>+ hash_for_each_possible(ldev->dmb_ht, tmp_node, list, dmb_tok) {
>+ if (tmp_node->token == dmb_tok) {
>+ rmb_node = tmp_node;
>+ break;
>+ }
>+ }
>+ if (!rmb_node) {
>+ read_unlock_bh(&ldev->dmb_ht_lock);
>+ return -EINVAL;
>+ }
>+ // So why copy the data now?? SMC usecase? Data buffer is attached,
>+ // rw-pointer are not attached?

I understand the confusion here. I have the same confusion the first time
I saw this.

This is actually the tricky part: it assumes the CDC will signal, while
the data will not. We need to copy the CDC, so the copy here only to the
CDC.

I think we should refine the move_data() API to make this clearer.

Best regards,
Dust