Shyam, comments will be posted shortly -- james On 7/30/2020 9:10 AM, Shyam Sundar wrote: > James, > > Please review the updated patchset to see if the changes make sense to you. > There are a few deviations from what we discussed here. > > I have used a single structure definition to maintain all fpin statistics > and we are maintain them at the granularity provided by the fabric instead of > Using a single counter per category. The same > structure is used across the fc_host_attr and the fc_rport structures. > > Given that the congestion signals will have to be maintained by the LLDs, > as we do not have the plumbing to push those notifications to the transport, > I have added them to the fc_host_statistics maintained and filled in by the > LLD. All of the stats are displayed under the "statistics" folder under the > sysfs. > > Under the host stats, a subset of stats (the Link Integrity FPINs) are > redundunt, and are already maintained, read via the traditional mechanism. > I have rolled them in nonetheless because during testing, they did not > update to the same values (using the FPIN error injection commands). If we > determine that these values will be in sync consistently, we can remove one > of the subset at a later point. > > Regards > Shyam > >> On Jun 25, 2020, at 4:25 PM, James Smart wrote: >> >> >> >> On 6/11/2020 10:42 AM, Shyam Sundar wrote: >>> Seems like this (and a previous email) never made it to the reflector, resending. >>> >>> The suggestions make sense to me, and I have made most of the recommended changes. >>> I was looking for some guidance on placements of the sim stats structures. >>> >>>> On May 29, 2020, at 11:53 AM, Shyam Sundar wrote: >>>> >>>> James, >>>> I was thinking of adding a structures for tracking the target FPIN stats. >>>> >>>> struct fc_rport_statistics { >>>> uint32_t link_failure_count; >>>> uint32_t loss_of_sync_count; >>>> .... >>>> } >>>> >>>> under fc_rport: >>>> >>>> struct fc_rport { >>>> >>>> /* Private (Transport-managed) Attributes */ >>>> struct fc_rport_statistics; >>>> >>>> For host FPIN stats (essentially the alarm & warning), was not sure if I should add them to the fc_host_statistics or >>>> define a new structure under the Private Attributes section within the fc_host_attrs/fc_vport. >> my initial thought was the same >> >>>> In theory, given that the host stats could be updated both via signals and FPIN, one could argue that it would make sense >>>> to maintain it with the current host statistics, but keeping it confined to transport will ensure we have a uniform way of handling >>>> congestion and peer congestion events. >> but then I have the same thoughts as well. And the commonization of the parsing and incrementing makes a lot more sense. >> >>>> Would appreciate your thoughts there. >>>> >>>> Thanks >>>> Shyam >>>> >> >> I would put them under the Dynamic attributes area in fc_host_attrs and fc_rport. >> fc_host_attrs: >> fpin_cn incremented for each Congestion Notify FPIN >> cn_sig_warn incremented for each congestion warning signal >> cn_sig_alarm incremented for each congestion alarm signal >> fpin_dn incremented for each Delivery Notification FPIN where attached wwpn is local port >> fpin_li incremented for each Link Integrity FPIN where attached wwpn is local port >> >> fc_rport: >> fpin_dn incremented for each Delivery Notification FPIN where attached wwpn is the rport >> fpin_li incremented for each Link Integrity FPIN where attached wwpn is the rport >> fpin_pcn incremented for each Peer Congestion Notify FPIN where attached wwpn is the rport >> >> For the cn_sig_xxx values, the driver would just increment them. >> For the fpin_xxx values - we'll augment the fc_host_fpin_rcv routine to parse the fpin and increment the fc_host or fc_rport counter. >> >> -- james