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
