On Mon, Jan 30, 2017 at 10:00 PM, Tom Herbert [off-list ref] wrote:

On Sun, Jan 29, 2017 at 12:07 AM, Saeed Mahameed
[off-list ref] wrote:

quoted

On Sat, Jan 28, 2017 at 7:19 PM, Tom Herbert [off-list ref] wrote:

quoted

On Sat, Jan 28, 2017 at 3:38 AM, Saeed Mahameed
[off-list ref] wrote:

quoted

On Fri, Jan 27, 2017 at 8:13 PM, Tom Herbert [off-list ref] wrote:

quoted

On Fri, Jan 27, 2017 at 9:58 AM, Saeed Mahameed
[off-list ref] wrote:

quoted

On Fri, Jan 27, 2017 at 1:32 AM, Tom Herbert [off-list ref] wrote:

quoted

This patchset creates configuration options for sriov, vxlan, eswitch,
and tc features in the mlx5 driver. The purpose of this is to allow not
building these features. These features are optional advanced features
that are not required for a core Ethernet driver. A user can disable
these features which resuces the amount of code in the driver. Disabling
these features (and DCB) reduces the size of mlx5_core.o by about 16%.
This is also can reduce the complexity of backport and rebases since
user would no longer need to worry about dependencies with the rest of
the kernel that features which might not be of any interest to a user
may bring in.

Tested: Build and ran the driver with all features enabled (the default)
and with none enabled (including DCB). Did not see any issues. I did
not explicity test operation of ayy of features in the list.

Basically I am not against this kind of change, infact i am with it,
although I would have done some restructuring in the driver before i
did such change ;), filling the code with ifdefs is not a neat thing.

If you wish, please take this as an RFC and feel free to structure the
code the right way. I think the intent is clear enough and looks like
davem isn't going to allow the directory restructuring so something
like this seems to be the best course of action now.

Right.

quoted

quoted

I agree this will simplify backporting and provide some kind of
feature separation inside the driver.
But this will also increase the testing matrix we need to cover and
increase the likelihood of kbuild breaks by an order of magnitude.

The testing matrix already exploded with the proliferation of
supported features. If anything this reduces the test matrix problem.
For instance, if we make a change to the core driver and functionality
properly isolated there is a much better chance that this won't affect
peripheral functionality and vice versa. It is just not feasible for
us to test every combination of NIC features for every change being
made.

Yes for isolated features, but for base functionality, we need to test
it with all new device specific kconfig combinations on every patch!

Sorry, but that is the price you need to pay for a feature rich device.

On the subject of testing, I don't really see any indication in these
patches on how patches are being tested. Also, there are patches that
fix things without any mention of how to repro the problems. It is

I Will do my best to provide more information in fixes commit
messages, Thanks for the input.

quoted

critical that we know IPv6 is tested as much or more than IPv4 (just

For the record, for every IPv4 test in our automatic regression system
we have an IPv6 equivalent test,
not to mention IPv6-only directed tests.

Great to know. Is there a publicly available description of what
specific tests are in the suite?

Nothing public, but i can collect some information and share it with
you if you wish.
but mostly traffic oriented tests and stress tests.
and configuration oriented tests:
  - basic configuration stuff: IPv6 with MTU/ring/offloads changes, etc..
  - Vxlan over IPv6, IPv6 over vxlan,
  - and some more

quoted

quoted

last week with hit yet another IPv6-only issue in an another upstream
driver that should have been caught with a simple load test-- this

Is there any IPv6-only functionality issues with mlx4/mlx5 that we
don't know about ?
If you do see any of those, please report them so we take the needed
corrective actions to fix them and cover them in our regression.

If we see them we will certainly let you know. This is not just
functionality either, performance regression versus IPv4 should also
be considered serious issues.

quoted

quoted

really is not acceptable any more!). Please add a description of how
patches were tested to commit logs.

Acknowledge.

Thanks,
Tom

quoted

quoted

Tom

quoted

since a misplaced code inside or outside the correct ifdef
can easily go unnoticed and break functionality.

quoted

quoted

One more thing, do we really need a device specific flag per feature
per vendor per device?  can't we just use the same kconfig flag for
all drivers and if there is a more generic system wide flag that
covers the same feature
can't we just use it, for instance instead of
CONFIG_<DRIVER_NAME>_SRIOV why not use already existing CONFIG_PCI_IOV
for all drivers ?

That sounds good to me. We already have CONFIG_RFS_ACCEL and others
that do that.

Tom

quoted

Saeed.

quoted

Tom Herbert (4):
  mlx5: Make building eswitch configurable
  mlx5: Make building SR-IOV configurable
  mlx5: Make building tc hardware offload configurable
  mlx5: Make building vxlan hardware offload configurable

 drivers/net/ethernet/mellanox/mlx5/core/Kconfig   |  35 ++++++
 drivers/net/ethernet/mellanox/mlx5/core/Makefile  |  16 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 129 ++++++++++++++++------
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c   |  39 +++++--
 drivers/net/ethernet/mellanox/mlx5/core/eq.c      |   4 +-
 drivers/net/ethernet/mellanox/mlx5/core/lag.c     |   2 +
 drivers/net/ethernet/mellanox/mlx5/core/main.c    |  32 ++++--
 drivers/net/ethernet/mellanox/mlx5/core/sriov.c   |   6 +-
 8 files changed, 205 insertions(+), 58 deletions(-)

--
2.9.3