On 9/8/2016 1:22 PM, Knut Omang wrote: > On Thu, 2016-09-08 at 07:33 -0400, Hal Rosenstock wrote: >> 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. > > Yes, with "handling" here we really mean "deciding what to do with it" - > in our case, the driver is not involved in those decisions at all. > That includes both inbound and outbound DR and LID routed SMInfo packets. > >>> 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 ? > > Yes, correct. > >> Do locally handled MADs work correctly ? > > Yes > >> Does OpenSM properly run on SIF? > > The goal is to have OpenSM work seamlessly with SIF, yes. More than whether it's a goal, I'm asking whether OpenSM currently works seamlessly with SIF with your changes (including driver not yet submitted to linux-rdma) ? >> 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. > > Do you have a link ref to this? http://lists.openfabrics.org/pipermail/users/2016-September/000607.html >>>>> 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. > > Ok, I see - good! > >>>>> 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 ? > > Yes, the driver have a work request based model to request such info from the device. > This is partly firmware based so it can be extended/adapted if necessary. sysfs.c could be extended to support this in addition to the current PMA approach for PortCounters/PortCountersExtended although PMA access to your device needs to work anyhow for at least PortCounters. Perhaps a new optional device driver entry point for get_port_stats taking ib_device struct and port number and returning some stats struct ? >>>> 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 ? > > Yes, that sounds about right, it's been a while since we struggled with this. > >> Doesn't the send MAD need to be tracked in the MAD layer ? > > The MAD layer tracking happens above the driver, it creates and manages QP0 and QP1 too > and sees all mad packets that gets sent as with Connect-X<n> ? Yes. That's what I'm referring to here as this is skipped since process_mad is NULL. Why wouldn't this be needed with SIF ? What happens if SIF device doesn't respond when it should ? Is there some error scenario where no response would come back ? -- Hal >> Is there some problem invoking ib_send_mad (or ib_send_rmpp_mad) ? > > No, to my knowledge we are able to support the normal management > applications just as one would expect. > > Thanks, > Knut > > -- 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
