IB/opa_vnic: Add debugfs interface

Hi Doug,

Here are the other two patches for opa_vnic. This adds a debugfs interface and that is why I'm sending it as a separate series apart from the rest of our submissions. The goal is to support view/modify of the VESW port config.

Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15

---

Quoted 10 lines · Dennis Dalessandro

Hi Doug,

Here are the other two patches for opa_vnic. This adds a debugfs interface and that is why I'm sending it as a separate series apart from the rest of our submissions. The goal is to support view/modify of the VESW port config.

Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15

---

Can you elaborate more on the reasons why debugfs was chosen over more generic configuration tool?

+DEBUGFS_FILE_OPS(vport_state);

Maybe, I'm too biased with rdmatool, but I don't see many reasons to introduce debugfs for the ULP.

Thanks

Quoted 17 lines · Leon Romanovsky

Quoted 10 lines · Dennis Dalessandro

Hi Doug,

Here are the other two patches for opa_vnic. This adds a debugfs interface and that is why I'm sending it as a separate series apart from the rest of our submissions. The goal is to support view/modify of the VESW port config.

Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15

---

Can you elaborate more on the reasons why debugfs was chosen over more generic configuration tool?

+DEBUGFS_FILE_OPS(vport_state);

Quoted 2 lines

Maybe, I'm too biased with rdmatool, but I don't see many reasons to introduce debugfs for the ULP.

And honestly, netdevices have so much tools to configure them (ethtool, iproute2, e.t.c.) so I completely don't understand the rationale behind debugfs for OPA-VNIC.

Thanks

Quoted 2 lines

Thanks

Quoted 18 lines · Leon Romanovsky

Quoted 17 lines · Leon Romanovsky

Quoted 10 lines · Dennis Dalessandro

Hi Doug,

Here are the other two patches for opa_vnic. This adds a debugfs interface and that is why I'm sending it as a separate series apart from the rest of our submissions. The goal is to support view/modify of the VESW port config.

Patches can can also be found in my GitHub repo at:
https://github.com/ddalessa/kernel/tree/for-4.15

---

Can you elaborate more on the reasons why debugfs was chosen over more generic configuration tool?

+DEBUGFS_FILE_OPS(vport_state);

Quoted 9 lines

Quoted 2 lines

Maybe, I'm too biased with rdmatool, but I don't see many reasons to introduce debugfs for the ULP.

And honestly, netdevices have so much tools to configure them (ethtool, iproute2, e.t.c.) so I completely don't understand the rationale behind debugfs for OPA-VNIC.

Thanks

Hi,

The debugfs interface here is for VNIC port encapsulation configuration. They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism. Hence, I think they probably shouldn't be in a tool like rdmatool.

Niranjana

Quoted 4 lines

Quoted 2 lines

Thanks

The debugfs interface here is for VNIC port encapsulation configuration.
They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism.

Quoted 6 lines · Niranjana

The debugfs interface here is for VNIC port encapsulation configuration.
They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism.

OK, if this isn't the supported configuration mechanism, then what is the supported mechanism?

-- >Doug Ledford <[email protected]>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Quoted 8 lines · Doug Ledford

Quoted 6 lines · Niranjana

The debugfs interface here is for VNIC port encapsulation configuration.
They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism.

Quoted 3 lines

OK, if this isn't the supported configuration mechanism, then what is the supported mechanism?

The supported mechanism is the Ethernet Manager providing the encapsulation configuration (as in the opa_vnic.txt documentation)
This is just a debug interface to it.

Niranjana

Quoted 4 lines

-- >Doug Ledford <[email protected]>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Quoted 13 lines · Niranjana

Quoted 11 lines · Doug Ledford

Quoted 6 lines · Niranjana

The debugfs interface here is for VNIC port encapsulation configuration.
They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism.

OK, if this isn't the supported configuration mechanism, then what is the supported mechanism?

Quoted 3 lines

The supported mechanism is the Ethernet Manager providing the encapsulation configuration (as in the opa_vnic.txt documentation)
This is just a debug interface to it.

Add tracepoint for that.

Quoted 7 lines

Niranjana

Quoted 4 lines

-- >Doug Ledford <[email protected]>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Quoted 14 lines · Leon Romanovsky

Quoted 13 lines · Niranjana

Quoted 11 lines · Doug Ledford

Quoted 6 lines · Niranjana

The debugfs interface here is for VNIC port encapsulation configuration.
They are not related to netdevice functionality.

They are for debug purpose only and not the supported configuration mechanism.

OK, if this isn't the supported configuration mechanism, then what is the supported mechanism?

Quoted 6 lines

Quoted 3 lines

The supported mechanism is the Ethernet Manager providing the encapsulation configuration (as in the opa_vnic.txt documentation)
This is just a debug interface to it.

Add tracepoint for that.

For some reason, I couldn't find Leon's earlier response on my mutt! Will repond here.

It is not really for tracing, but to have a simple debug read/write access to the encapsution configuration which allows us to triage issues.

We find it useful on an ongoing basis, more like some standard NICs providing debugfs interface to read/write from/to some of its registers.

It is much easier to have it upstreamed and available instead of maintaining these patches separately.

Niranjana

Quoted 7 lines

Quoted 7 lines

Niranjana

Quoted 4 lines

-- >Doug Ledford <[email protected]>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD

Quoted 2 lines

We find it useful on an ongoing basis, more like some standard NICs providing debugfs interface to read/write from/to some of its registers.

Usually if such interface is spotted during submission, it will be requested to drop it. Ask Salil, who lately upstreamed new ETH driver and was asked to remove it.

Quoted 3 lines · Leon Romanovsky

Quoted 2 lines

We find it useful on an ongoing basis, more like some standard NICs providing debugfs interface to read/write from/to some of its registers.

Quoted 3 lines

Usually if such interface is spotted during submission, it will be requested to drop it. Ask Salil, who lately upstreamed new ETH driver and was asked to remove it.

To be fair Salil's was dropped more or less for security reasons..

I think debugfs to dump truely internal state that is not intended to be API stable is reasonable.

Promise you will not be tooling on top of this, or use it as an interface in your manager or something crazy like that. You must be happy in all cases if your distro chooses to disable debugfs.

The problem I have with reviewing all the vnic patches is that I don't have a spec for vnic or anything, and I don't really know what the data being dumped is..

eg I would have been much happier if ipoib
dump'd its debugfs stuff through netlink connected to neighbor objects
- I don't know if this is something similar...

As Leon says, you should really think carefully if this needs to be something that admins are going to be told to look at when vnic isn't working properly, as the ipoib debufs is. In that case netlink is a stable API and more suitable.

Jason

Quoted 9 lines · Jason Gunthorpe

Quoted 6 lines · Leon Romanovsky

Quoted 2 lines

We find it useful on an ongoing basis, more like some standard NICs providing debugfs interface to read/write from/to some of its registers.

Usually if such interface is spotted during submission, it will be requested to drop it. Ask Salil, who lately upstreamed new ETH driver and was asked to remove it.

Quoted 4 lines

To be fair Salil's was dropped more or less for security reasons..

I think debugfs to dump truely internal state that is not intended to be API stable is reasonable.

Yes, it can be ripped in any point of life, despite the screaming.

Quoted 3 lines

Promise you will not be tooling on top of this, or use it as an interface in your manager or something crazy like that. You must be happy in all cases if your distro chooses to disable debugfs.

Honestly, I don't know if we can trust to Intel's promises.

Among other promises, they wrote that OmniPath Ethernet Management part will be open sourced too. It seems like it wasn't at the end.
My google searches with words "ethernet manager omni-path" revealed nothing.

Quoted 3 lines

The problem I have with reviewing all the vnic patches is that I don't have a spec for vnic or anything, and I don't really know what the data being dumped is..

My problem with those patches that they don't show whole picture and the more important you have no way to see it, if you want it.
Right now, it is legal code-sharing and not truly open-source which I would like to see.

Quoted 3 lines

eg I would have been much happier if ipoib
dump'd its debugfs stuff through netlink connected to neighbor objects
- I don't know if this is something similar...

Quoted 4 lines

As Leon says, you should really think carefully if this needs to be something that admins are going to be told to look at when vnic isn't working properly, as the ipoib debufs is. In that case netlink is a stable API and more suitable.

All of this vnic stuff is a work in progress and we are incrementally pushing things out. Iterative development, involving the community and all.

Quoted 3 lines · Dennis Dalessandro

All of this vnic stuff is a work in progress and we are incrementally pushing things out. Iterative development, involving the community and all.

Can we get OPA-VNIC specification? Can we get OPA specification?

The Documentation/infiniband/opa_vnic.txt is not enough to really implement anything on top of that or communicate with that.

For example, what are the values and relations for routing control? How is entropy calculated?

Thanks

You are putting a wrong interpretation to this patch.
EM provides the encapsulation configuration as in the opa_vnic documentation. EM will be opensourced when it is ready (VNIC is development in progress).

As mentioned, this patch is a simple *debug* hook to that encapsulation configuration and no more.
That is why it is put under DEBUGFS.

I do see valid reason in Jason's and your earlier comments that it should a formal support to the admins.
But this patch was not mainly for that. I don't expect DEBUGFS to be turned on in all deployments.
I understand the question whether or not this patch should be in the kernel tree.
But it is a useful debug hook for debugging/triaging issues (similar to NIC having debug hook read/write some register), and convenient to have it available in the kernel than maintaining it separately.

Niranjana

Quoted 1 line

Thanks

Upstream is not a development playground and you should submit your code once you think that it is ready. So we are assuming that VNIC is working and you are interested to debug your EM and such code doesn't belong to the kernel.

If you still insist on 2.a, the solution should be in your company: add debugfs locally, write tests, find and fix bugs and submit them to upstream.

There is no need to add "one-time" interface to clean the code.

Quoted 3 lines · Leon Romanovsky

Upstream is not a development playground and you should submit your code once you think that it is ready. So we are assuming that VNIC is working and you are interested to debug your EM and such code doesn't belong to the kernel.

This patch aside, I do have to take some issue with this statement. While upstream is not a "playground" we should be submitting code early. I keep harping on iterative development, show your work. Granted that doesn't mean write broken code and toss it over the fence to kernel.org, but pieces that can be broken up and tested on their own are best.

Quoted 4 lines

If you still insist on 2.a, the solution should be in your company: add debugfs locally, write tests, find and fix bugs and submit them to upstream.

Now back to this patch series. debugfs vs NetLink, I don't think it really matters if the rationale for having the patch in the first place is wrong. In other words you would have still opposed this even if it were NetLink I assume? We are discussing internally but for now I think this series can be dropped.

Quoted 2 lines

There is no need to add "one-time" interface to clean the code.

That I would agree to.

-Denny

Quoted 10 lines · Dennis Dalessandro

Quoted 3 lines · Leon Romanovsky

Upstream is not a development playground and you should submit your code once you think that it is ready. So we are assuming that VNIC is working and you are interested to debug your EM and such code doesn't belong to the kernel.

This patch aside, I do have to take some issue with this statement. While upstream is not a "playground" we should be submitting code early. I keep harping on iterative development, show your work. Granted that doesn't mean write broken code and toss it over the fence to kernel.org, but pieces that can be broken up and tested on their own are best.

We are talking about the same. Large feature (VNIC) was divided to sub-tasks like patches and maybe sub-features. Those sub-features were submitted early (iterative development), which is good and sounds right for me too.

But the expectation is that these building blocks are tested and working and you don't need to debug them.

For sure, there are always exceptions to the statements above and various counters, states, dumps e.t.c are good examples for such exceptions, because they add visibility to the system, but they don't change the system.

Quoted 10 lines

Quoted 4 lines

If you still insist on 2.a, the solution should be in your company: add debugfs locally, write tests, find and fix bugs and submit them to upstream.

Now back to this patch series. debugfs vs NetLink, I don't think it really matters if the rationale for having the patch in the first place is wrong. In other words you would have still opposed this even if it were NetLink I assume? We are discussing internally but for now I think this series can be dropped.

I am more than happy to see OPA-VNIC configurations in NetLink, but don't forget that it will be part of ABI and users will use it, so you need to ensure that the OPA-VNIC network won't die after one of the nodes will change vport value.

Before you are rushing to implement it via netlink, you should ask yourself WHY this functionality is needed for the users.