Re: [RFC net-next 0/7] Provide an ism layer

[Dust Li <dust.li@linux.alibaba.com>]

On 2025-01-17 12:04:06, Alexandra Winter wrote:
>I hit the send button to early, sorry about that. 
>Let me comment on the other proposals from Dust Li as well.
>
>On 16.01.25 10:32, Dust Li wrote:
>> Abstraction of ISM Device Details: I propose we abstract the ISM device
>> details by providing SMC with helper functions. These functions could
>> encapsulate ism->ops, making the implementation cleaner and more
>> intuitive. 
>
>
>Maybe I misunderstand what you mean by helper functions..
>Why would you encapsulate ism->ops functions in another set of wrappers?
>I was happy to remove the helper functions in 2/7 and 7/7.

What I mean is similar to how IB handles it in include/rdma/ib_verbs.h.
A good example is ib_post_send or ibv_post_send in user space:

```c
static inline int ib_post_send(struct ib_qp *qp,
                               const struct ib_send_wr *send_wr,
                               const struct ib_send_wr **bad_send_wr)
{
        const struct ib_send_wr *dummy;

        return qp->device->ops.post_send(qp, send_wr, bad_send_wr ? : &dummy);
}
```

By following this approach, we can "hide" all the implementations behind
ism_xxx. Our users (SMC) should only interact with these APIs. The ism->ops
would then be used by our device implementers (vISM, loopback, etc.). This
would help make the layers clearer, which is the same approach IB takes.

The layout would somehow like this:

| -------------------- |-----------------------------|
|  ism_register_dmb()  |                             |
|  ism_move_data()     | <---  API for our users     |
|  ism_xxx() ...       |                             |
| -------------------- |-----------------------------|
|   ism_device_ops     | <---for our implementers    |
|                      |    (PCI-ISM/loopback, etc)  |
|----------------------|-----------------------------|


>
>
>This way, the struct ism_device would mainly serve its
>> implementers, while the upper helper functions offer a streamlined
>> interface for SMC.
>
>
>I was actually also wondering, whether the clients should access ism_device
>at all. Or whether they should only use the ism_ops.

I believe the client should only pass an ism_dev pointer to the ism_xxx()
helper functions. They should never directly access any of the fields inside
the ism_dev.


>I can give that a try in the next version. I think this RFC almost there already.
>The clients would still need to pass a poitner to ism_dev as a parameter.
>
>
>> Structuring and Naming: I recommend embedding the structure of ism_ops
>> directly within ism_dev rather than using a pointer. 
>
>
>I think it is a common method to have the const struct xy_ops in the device driver code
>and then use pointer to register the device with an upper layer.

Right, If we have many ism_devs for each one ISM type, then using pointer
should save us some memory.

>What would be the benefit of duplicating that struct in every ism_dev?

The main benefit of embedding ism_device_ops within ism_dev is that it
reduces the dereferencing of an extra pointer. We already have too many
dereference in the datapath, it is not good for performance :(

For example:

rc = smcd->ism->ops->move_data(smcd->ism, dmb_tok, idx, sf, offset,
                               data, len);

Best regards,
Dust