Re: [PATCH RFC] RDMA/core: add rdma_get_dma_mr()

Steve Wise
2015-06-26T15:18:55Z

On 6/26/2015 9:02 AM, Steve Wise wrote:
> On 6/25/2015 5:37 PM, Hefty, Sean wrote:
>>> +enum rdma_mr_roles {
>> I would drop naming the enum - it shouldn't be used, as the values
>> are bit flags.
>
> ok.
>
>>> + RDMA_MRR_RECV = 1,
>>> + RDMA_MRR_SEND = (1<<1),
>>> + RDMA_MRR_READ_SOURCE = (1<<2),
>>> + RDMA_MRR_READ_SINK = (1<<3),
>>> + RDMA_MRR_WRITE_SOURCE = (1<<4),
>>> + RDMA_MRR_WRITE_SINK = (1<<5),
>>> + RDMA_MRR_ATOMIC = (1<<6),
>>> + RDMA_MRR_MW_BIND = (1<<7),
>>> + RDMA_MRR_ZERO_BASED = (1<<8),
>> There's 'something' different about this role that cause me
>> hesitation. Maybe that it's dependent on other roles being set to be
>> useful? I'm not sure.
>>
>> Maybe we need both roles and registration flags, with this declared
>> as a flag?
>>
>>> + RDMA_MRR_ACCESS_ON_DEMAND = (1<<9),
>> This one is even more different, as it doesn't impact how the MR
>> interacts with the interfaces, or change how the application uses the
>> MR. This is really a hint to the provider regarding the selection of
>> different implementation flows.
>
> How about roles and attributes? ZERO_BASED and ACCESS_ON_DEMAND would
> be attributes and the rest roles.
>

I'm thinking now about the access flags specified in a IB_WR_FAST_REG_MR
work request. I think we need a similar function for this, but it will
return the ib_access_flags bits to be stored in
ib_send_wr->wr.fast_reg.access_flags field. With this function defined,
then rdma_get_dma_mr() can use it as well.

So perhaps an internal static function in verbs.c that does the bulk of
what I put in rdma_get_dma_mr() originally called:

device_mr_access_flags(device, roles, attrs)

used by exported functions (or inlines in ib_verbs.h):

rdma_get_dma_mr(device, roles, attrs)
rdma_fast_reg_access_flags(device, roles, attrs)

I'll code this up and send for another round of review. Eventually i'll
get this included in the series with the iSER/iWARP patches for
testing/further review.

Steve.