On Wed, Apr 14, 2021 at 10:16:28AM -0400, Tom Talpey wrote: > On 4/12/2021 6:48 PM, Jason Gunthorpe wrote: > > On Mon, Apr 12, 2021 at 04:20:47PM -0400, Tom Talpey wrote: > > > > > So the issue is only in testing all the providers and platforms, > > > to be sure this new behavior isn't tickling anything that went > > > unnoticed all along, because no RDMA provider ever issued RO. > > > > The mlx5 ethernet driver has run in RO mode for a long time, and it > > operates in basically the same way as RDMA. The issues with Haswell > > have been worked out there already. > > > > The only open question is if the ULPs have errors in their > > implementation, which I don't think we can find out until we apply > > this series and people start running their tests aggressively. > > I agree that the core RO support should go in. But turning it on > by default for a ULP should be the decision of each ULP maintainer. > It's a huge risk to shift all the storage drivers overnight. How > do you propose to ensure the aggressive testing happens? Realistically we do test most of the RDMA storage ULPs at NVIDIA over mlx5 which is the only HW that will enable this for now. I disagree it is a "huge risk". Additional wider testing is welcomed and can happen over the 16 week release cycle for a kernel. I would aim to get the relaxed ordering changed merged to linux-next a week or so after the merge window. Further testing happens before these changes would get picked up in a distro on something like MLNX_OFED. I don't think we need to make the patch design worse or over think the submission process for something that, so far, hasn't discovered any issues and alread has a proven track record in other ULPs. Any storage ULP that has a problem here is mis-using verbs and the DMA API and thus has an existing data-corruption bug that they are simply lucky to have not yet discovered. > One thing that worries me is the patch02 on-by-default for the dma_lkey. > There's no way for a ULP to prevent IB_ACCESS_RELAXED_ORDERING > from being set in __ib_alloc_pd(). The ULPs are being forced into relaxed_ordering. They don't get to turn it off one by one. The v2 will be more explicit about this as there will be no ULP patches, just the verbs core code being updated. Jason
