Date:	Tue, 07 Apr 2015 18:05:18 +0200
From:	Michael Wang <yun.wang@...fitbricks.com>
To:	Tom Talpey <tom@...pey.com>, Roland Dreier <roland@...nel.org>,
	Sean Hefty <sean.hefty@...el.com>, linux-rdma@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
	netdev@...r.kernel.org
CC:	Hal Rosenstock <hal.rosenstock@...il.com>,
	Tom Tucker <tom@...ngridcomputing.com>,
	Steve Wise <swise@...ngridcomputing.com>,
	Hoang-Nam Nguyen <hnguyen@...ibm.com>,
	Christoph Raisch <raisch@...ibm.com>,
	Mike Marciniszyn <infinipath@...el.com>,
	Eli Cohen <eli@...lanox.com>,
	Faisal Latif <faisal.latif@...el.com>,
	Upinder Malhi <umalhi@...co.com>,
	Trond Myklebust <trond.myklebust@...marydata.com>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ira Weiny <ira.weiny@...el.com>,
	PJ Waskiewicz <pj.waskiewicz@...idfire.com>,
	Tatyana Nikolova <Tatyana.E.Nikolova@...el.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Jack Morgenstein <jackm@....mellanox.co.il>,
	Haggai Eran <haggaie@...lanox.com>,
	Ilya Nelkenbaum <ilyan@...lanox.com>,
	Yann Droneaud <ydroneaud@...eya.com>,
	Bart Van Assche <bvanassche@....org>,
	Shachar Raindel <raindel@...lanox.com>,
	Sagi Grimberg <sagig@...lanox.com>,
	Devesh Sharma <devesh.sharma@...lex.com>,
	Matan Barak <matanb@...lanox.com>,
	Moni Shoua <monis@...lanox.com>, Jiri Kosina <jkosina@...e.cz>,
	Selvin Xavier <selvin.xavier@...lex.com>,
	Mitesh Ahuja <mitesh.ahuja@...lex.com>,
	Li RongQing <roy.qing.li@...il.com>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Alex Estrin <alex.estrin@...el.com>,
	Doug Ledford <dledford@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Erez Shitrit <erezsh@...lanox.com>,
	Tom Gundersen <teg@...m.no>,
	Chuck Lever <chuck.lever@...cle.com>
Subject: Re: [PATCH v2 09/17] IB/Verbs: Use helper cap_read_multi_sge() and
 reform svc_rdma_accept()

Hi, Tom

Thanks for the comments :-)

On 04/07/2015 05:46 PM, Tom Talpey wrote:
> On 4/7/2015 8:34 AM, Michael Wang wrote:
>>   /**
>> + * cap_read_multi_sge - Check if the port of device has the capability
>> + * RDMA Read Multiple Scatter-Gather Entries.
>> + *
>> + * @device: Device to be checked
>> + * @port_num: Port number of the device
>> + *
>> + * Return 0 when port of the device don't support
>> + * RDMA Read Multiple Scatter-Gather Entries.
>> + */
>> +static inline int cap_read_multi_sge(struct ib_device *device, u8 port_num)
>> +{
>> +    return !rdma_transport_iwarp(device, port_num);
>> +}
> 
> This just papers over the issue we discussed earlier. How *many*
> entries does the device support? If a device supports one, or two,
> is that enough? How does the upper layer know the limit?
> 
> This needs an explicit device attribute, to be fixed properly.

This is the prototype to expose the problem we have in here, I
would prefer some one good at this part to extending the API in
future, basing on the right logical.

Currently this just inherit from the legacy, it implemented
in order to be compatible with the current code.

> 
>> +
>> +/**
>>    * cap_ipoib - Check if the port of device has the capability
>>    * IP over Infiniband.
>>    *
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> index e011027..604d035 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> @@ -118,8 +118,8 @@ static void rdma_build_arg_xdr(struct svc_rqst *rqstp,
>>
>>   static int rdma_read_max_sge(struct svcxprt_rdma *xprt, int sge_count)
>>   {
>> -    if (rdma_node_get_transport(xprt->sc_cm_id->device->node_type) ==
>> -         RDMA_TRANSPORT_IWARP)
>> +    if (!cap_read_multi_sge(xprt->sc_cm_id->device,
>> +                xprt->sc_cm_id->port_num))
>>           return 1;
>>       else
>>           return min_t(int, sge_count, xprt->sc_max_sge);
> 
> This is incorrect. The RDMA Read max is not at all the same as the
> max_sge. It is a different operation, with a different set of work
> request parameters.
> 
> In other words, the above same comment applies.

Any idea on how to improve this part?

Again, all these helpers just inherit the old logical, if
it's wrong, let's correct it ;-)

And if we don't know how to correct, we can leave this as a
signpost and waiting for someone good at this particular part
to fix it.

> 
> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 4e61880..e75175d 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -979,8 +979,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
>>       /*
>>        * Determine if a DMA MR is required and if so, what privs are required
>>        */
>> -    switch (rdma_node_get_transport(newxprt->sc_cm_id->device->node_type)) {
>> -    case RDMA_TRANSPORT_IWARP:
>> +    if (rdma_transport_iwarp(newxprt->sc_cm_id->device,
>> +                 newxprt->sc_cm_id->port_num)) {
>>           newxprt->sc_dev_caps |= SVCRDMA_DEVCAP_READ_W_INV;
> 
> Do I read this correctly that it is forcing the "read with invalidate"
> capability to "on" for all iWARP devices? I don't think that is correct,
> for the legacy devices you're also supporting.

Hmm.. but that's exactly same as the old logical, correct?
Or do you mean the old logical is wrong?

> 
> 
>> @@ -992,8 +992,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
>>               dma_mr_acc = IB_ACCESS_LOCAL_WRITE;
>>           } else
>>               need_dma_mr = 0;
>> -        break;
>> -    case RDMA_TRANSPORT_IB:
>> +    } else if (rdma_ib_mgmt(newxprt->sc_cm_id->device,
>> +                newxprt->sc_cm_id->port_num)) {
>>           if (!(newxprt->sc_dev_caps & SVCRDMA_DEVCAP_FAST_REG)) {
>>               need_dma_mr = 1;
>>               dma_mr_acc = IB_ACCESS_LOCAL_WRITE;
> 
> Now I'm even more confused. How is the presence of IB management
> related to needing a privileged lmr?

I think you actually mean we need some more wrapper here
with the right name, correct?

I'm not good at this part, any suggestions?

Regards,
Michael Wang

> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists