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
Jes, we will do the exercise, there will be a config directive for TC offloads and another one for eswitch/sriov support. Or.
