On 9/7/2016 7:42 AM, Knut Omang wrote:
> On Tue, 2016-09-06 at 10:01 -0400, Hal Rosenstock wrote:
>> On 9/1/2016 8:09 PM, Knut Omang wrote:
>>>
>>> From: Dag Moxnes <dag.moxnes@xxxxxxxxxx>
>>>
>>> The process_mad function is an optional IB driver entry point
>>> allows a driver to intercept or modify MAD traffic.
>>
>> process_mad is optional but currently that optionality is based on
>> whether MADs (QP0 and/or QP1) are supported or not. This is reflected in
>> the core_cap_flags and is related to the device type: IB including RoCE
>> v. non IB. The current in tree device drivers that do not support
>> process_mad are i40iw (iWARP) and usnic.
>
> I see - we are introducing a new case here, then.
>>>
>>> This fix allows MAD traffic to flow down to the device also
>>> when MAD traffic is completely handled by the device
>>
>> All MAD traffic is not handled by the device.
>
> Yes, in our case all MAD packet handling is done by the device,
SMInfo is not handled by device unless you have SM in hardware.
It also depends on management class, method, and direction
(outgoing/incoming) and in case of DR SMP the DR path.
> the driver's task for MAD packets is just to forward.
I think that it's a little more complex than that. See above.
Perhaps some of this is due to not understanding what the effects of
forwarding are with device such as SIF where SMA (and SMI) as well as
PMA are totally in hardware. If MAD is locally handled rather than
"forwarded", I assume that the device generates and sends the response
MAD, right ?
Do locally handled MADs work correctly ? Does OpenSM properly run on SIF
? I saw a post yesterday on issue with running OpenSM on SIF in terms of
obtaining the local PortInfo via DR SMP with hop count of 0.
>>>
>>> and no process_mad function is provided.
>>>
>>> Signed-off-by: Knut Omang <knut.omang@xxxxxxxxxx>
>>> ---
>>> drivers/infiniband/core/mad.c | 6 ++++++
>>> drivers/infiniband/core/smi.h | 6 ++----
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
>>> index 2d49228..ece33ec 100644
>>> --- a/drivers/infiniband/core/mad.c
>>> +++ b/drivers/infiniband/core/mad.c
>>> @@ -813,6 +813,12 @@ static int handle_outgoing_dr_smp(struct ib_mad_agent_private *mad_agent_priv,
>>> goto out;
>>> }
>>>
>>> + /* If device does not define the optional process_mad function it means that
>>> + * everything is handled in hardware:
>>> + */
>>> + if (!device->process_mad)
>>> + goto out;
>>> +
>> This changes the ib_post_send_mad flow for those devices in that it
>> takes the send rather than error path.
>
> Yes, but no packets will ever be received by this function for the i40iw and usnic
> devices because they have said in their capabilities that they do not support SMI?
You're right - it shouldn't get here for those devices (it's based on
MAD rather than SMI cap) - umad open would fail for the port GUID.
>>> local = kmalloc(sizeof *local, GFP_ATOMIC);
>>> if (!local) {
>>> ret = -ENOMEM;
>>> diff --git a/drivers/infiniband/core/smi.h b/drivers/infiniband/core/smi.h
>>> index 33c91c8..16a9f9a 100644
>>> --- a/drivers/infiniband/core/smi.h
>>> +++ b/drivers/infiniband/core/smi.h
>>> @@ -67,8 +67,7 @@ static inline enum smi_action smi_check_local_smp(struct ib_smp *smp,
>>> {
>>> /* C14-9:3 -- We're at the end of the DR segment of path */
>>> /* C14-9:4 -- Hop Pointer = Hop Count + 1 -> give to SMA/SM */
>>> - return ((device->process_mad &&
>>> - !ib_get_smp_direction(smp) &&
>>> + return ((!ib_get_smp_direction(smp) &&
>>> (smp->hop_ptr == smp->hop_cnt + 1)) ?
>>> IB_SMI_HANDLE : IB_SMI_DISCARD);
>>> }
>>> @@ -82,8 +81,7 @@ static inline enum smi_action smi_check_local_returning_smp(struct ib_smp *smp,
>>> {
>>> /* C14-13:3 -- We're at the end of the DR segment of path */
>>> /* C14-13:4 -- Hop Pointer == 0 -> give to SM */
>>> - return ((device->process_mad &&
>>> - ib_get_smp_direction(smp) &&
>>> + return ((ib_get_smp_direction(smp) &&
>>> !smp->hop_ptr) ? IB_SMI_HANDLE : IB_SMI_DISCARD);
>>> }
>> Also, with this approach of optional process_mad for IB device, will/how
>> will sysfs port counters be supported for this device ? This currently
>> relies on process_mad.
>
> Yes, that is actually a problem for us right now.
> We do however think that the solution of composing a packet to send via process_mad is
> kind of overkill as this doesn't allow devices to optimize the way to retrieve this info.
Is there a way other than via PMA to get these counters ?
>> It should be possible to implement a process_mad routine to return
>> ib_mad_result based on management class and perhaps attribute ID in the
>> case of SMInfo. Can this alternative approach be used for SIF ?
>
> The problem is that the handle_outgoing_dr_smp function has an implicit assumption that
> some packets are handled by the process_mad function itself. There is no way to provide return
> values from the process_mad function that ensures that packets are always forwarded to the device,
> so the only viable solution without breaking the API seems to be to not implement process_mad.
Are you referring to the difference between returning 0 when no
process_mad function as this patch does and what happens when
process_mad returns 0 inside of the ib_mad_post_send routine ? Doesn't
the send MAD need to be tracked in the MAD layer ? Is there some problem
invoking ib_send_mad (or ib_send_rmpp_mad) ?
-- Hal
> Thanks for the review,
>
> Knut
>
>> -- Hal
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
