Date:   Mon, 30 Jan 2017 23:26:22 +0200
From:   Saeed Mahameed <saeedm@....mellanox.co.il>
To:     Tom Herbert <tom@...bertland.com>
Cc:     Saeed Mahameed <saeedm@...lanox.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Netdev List <netdev@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH net-next 0/4] mlx5: Create build configuration options

On Mon, Jan 30, 2017 at 10:00 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Sun, Jan 29, 2017 at 12:07 AM, Saeed Mahameed
> <saeedm@....mellanox.co.il> wrote:
>> On Sat, Jan 28, 2017 at 7:19 PM, Tom Herbert <tom@...bertland.com> wrote:
>>> On Sat, Jan 28, 2017 at 3:38 AM, Saeed Mahameed
>>> <saeedm@....mellanox.co.il> wrote:
>>>> On Fri, Jan 27, 2017 at 8:13 PM, Tom Herbert <tom@...bertland.com> wrote:
>>>>> On Fri, Jan 27, 2017 at 9:58 AM, Saeed Mahameed
>>>>> <saeedm@....mellanox.co.il> wrote:
>>>>>> On Fri, Jan 27, 2017 at 1:32 AM, Tom Herbert <tom@...bertland.com> wrote:
>>>>>>> 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.
>>>>
>>>>>> 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.
>>
>>> 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

>>> 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.
>
>>> really is not acceptable any more!). Please add a description of how
>>> patches were tested to commit logs.
>>>
>>
>> Acknowledge.
>>
>
> Thanks,
> Tom
>
>>> Tom
>>>
>>>> since a misplaced code inside or outside the correct ifdef
>>>> can easily go unnoticed and break functionality.
>>>>
>>>>>> 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
>>>>>
>>>>>> Saeed.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>>

Powered by blists - more mailing lists