On Sat, Jun 3, 2017 at 10:37 PM, Or Gerlitz <gerlitz...@gmail.com> wrote: > On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsoren...@fb.com> wrote: >> On 05/28/2017 02:03 AM, Or Gerlitz wrote: >>> >>> On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.soren...@gmail.com> >>> wrote: >>>> >>>> On 05/27/2017 05:02 PM, Or Gerlitz wrote: >>>>> >>>>> >>>>> On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.soren...@gmail.com> >>>>> wrote: >>>>>> >>>>>> >>>>>> This gets rid of the temporary #ifdef spaghetti and allows the code to >>>>>> compile without offload support enabled. >>> >>> >>>>> I am pretty sure we can do that exercise you're up to without any >>>>> spaghetti cooking and even put more code under that CONFIG directive >>>>> (en_rep.c), I'll take that with Saeed. >>> >>> >>>> I want to avoid adding #ifdef CONFIG_foo to the main code in order to >>>> keep >>>> it readable. I did it gradually to make sure I didn't break anything and >>>> to >>>> allow for it to be bisected in case something did break. If we can move >>>> out >>>> more code from places like en_rep.c into eswitch_offload.c and get it >>>> disabled that way that would be great, but I like to limit the number of >>>> #ifdefs we add to the actual code. >>> >>> >>> FWIW (see below), squashing your seven patches to one resulted in a >>> fairly simple/clear >>> patch, so if we go that way, no need to have seven commits just for this >>> piece. >> >> >> Squashing patches into jumbo patches is inherently broken and bad coding >> practice! It makes it way more complicated to debug and bisect in case a >> minor detail broke in the process. > > Not that pure LOC ##-s is the only/deep measurement, but your overall > changes in the the seven patch series account to: > > 5 files changed, 94 insertions(+), 3 deletions(-) > > and by no mean this is jumbo or inherently broken and bad coded, so > please slow down please, I looked with care on the resulted patch and > said it's basically ok. > > >>> Re SRIOV, I don't think it would be correct to break the support info few >>> CONFIG directives. If we want to allow someone to build the driver w.o >>> SRIOV that's fine, but I don't think we should further go down and disable >>> some of the SRIOV sub-modes. > >>> Re TC offload support, that's make sense. > >> OK, so disabling SRIOV and disabling TC makes sense - I'll look at that. > > I think Saeed wants us to conduct that exercise, let me check with him > and get back to you
disabling SRIOV in all the kernel can do the trick, but we want something more flexible, yet simple. eswitch is required __ONLY__ for SRIOV, in the light of that fact we can have CONFIG_MLX5_SRIOV which depends on kernel SRIOV kconfig directive, that will eliminate MLX5 sriov support (basically compile out sriov.c, eswitch.c, eswitch_offloads.c and en_rep.c). and stub out some sriov NDOs and some little API calls from the main flows to the above mlx5 sub-modules. Also we will need to compile out some en_tc.c offloads which meant to program the eswitch they also call the eswitch_offloads API.
