| Message ID | 20170526211624.23133-8-jsorensen@fb.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote: > This gets rid of the temporary #ifdef spaghetti and allows the code to > compile without offload support enabled. Hi Jes, 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. Just wondering, you are motivated by a wish to put some mlx5 functionalities under their own CONFIG directives which could be useful when backporting the latest upstream driver into older kernel and being able not to deal with parts of it, right? in that respect, are you using SRIOV but not the offloads mode? Or.
On 05/27/2017 05:02 PM, Or Gerlitz wrote: > On Sat, May 27, 2017 at 12:16 AM, Jes Sorensen <jes.sorensen@gmail.com> wrote: >> This gets rid of the temporary #ifdef spaghetti and allows the code to >> compile without offload support enabled. > > Hi Jes, > > 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. Hi Or, 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. > Just wondering, you are motivated by a wish to put some mlx5 > functionalities under their own CONFIG directives which could be > useful when backporting the latest upstream driver into older kernel > and being able not to deal with parts of it, right? in that respect, > are you using SRIOV but not the offloads mode? The motivation is two-fold, the primary is to be able to disable features not being used for those who compile a custom kernel and who wish to reduce the codebase compiled. It also makes it more flexible when back porting the code to older kernels since it is easier to pick out a smaller subset. I was going to look into making TC support etc. optional next, but I wanted to have a discussion about this patchset first. Cheers, Jes
On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@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.sorensen@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. >> Just wondering, you are motivated by a wish to put some mlx5 >> functionalities under their own CONFIG directives which could be >> useful when backporting the latest upstream driver into older kernel >> and being able not to deal with parts of it, right? in that respect, >> are you using SRIOV but not the offloads mode? > The motivation is two-fold, the primary is to be able to disable features > not being used for those who compile a custom kernel and who wish to reduce > the codebase compiled. It also makes it more flexible when back porting the > code to older kernels since it is easier to pick out a smaller subset. I was > going to look into making TC support etc. optional next, but I wanted to > have a discussion about this patchset first. OKay, I got you. 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. Or.
On 05/28/2017 02:03 AM, Or Gerlitz wrote: > On Sun, May 28, 2017 at 5:23 AM, Jes Sorensen <jes.sorensen@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.sorensen@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. >>> Just wondering, you are motivated by a wish to put some mlx5 >>> functionalities under their own CONFIG directives which could be >>> useful when backporting the latest upstream driver into older kernel >>> and being able not to deal with parts of it, right? in that respect, >>> are you using SRIOV but not the offloads mode? > >> The motivation is two-fold, the primary is to be able to disable features >> not being used for those who compile a custom kernel and who wish to reduce >> the codebase compiled. It also makes it more flexible when back porting the >> code to older kernels since it is easier to pick out a smaller subset. I was >> going to look into making TC support etc. optional next, but I wanted to >> have a discussion about this patchset first. > > OKay, I got you. > > 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. Jes
On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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
On Sat, Jun 3, 2017 at 10:37 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote: > On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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.
On Sat, Jun 3, 2017 at 10:37 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote: > On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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.
On 06/03/2017 03:37 PM, Or Gerlitz wrote: > On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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. Squashing patches for the sake of squashing patches is inherently broken and bad. So please calm down and stop this mangling of other peoples' patches. If you want an alternative, put up a proposal and look at it for comparison somewhere. Jes
On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote: > On 06/03/2017 03:37 PM, Or Gerlitz wrote: >> >> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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. > > > Squashing patches for the sake of squashing patches is inherently broken and > bad. So please calm down and stop this mangling of other peoples' patches. > > If you want an alternative, put up a proposal and look at it for comparison > somewhere. > > Jes > Hey Jes, It is not just about squashing patches, I am working on a series of patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc altogether, it will come out cleaner as it will remove all ethernet sriov/eswitch VF representors and eswitch tc offloads stuff with one kconfig flag, and yet preserve standard QoS functionality from en_tc. BTW today you can just remove eswitch from driver and non sriov configuration will perfectly work with no issues. Even multi PF configuration will also work, but without l2 mac table, which means PFs can only see packets with their own static (permanent) mac addresses, user configured macs will not work on Multi PF configuration. For that i will take the l2 table (ConnectX PF mac table) logic out of eswitch as it is not really an eswitch logic, and move it to core driver to allow Multi PF configuration to work without eswitch. I will post some patches for you to review by end of week. Thanks, Saeed.
On 06/05/2017 05:53 PM, Saeed Mahameed wrote: > On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote: >> On 06/03/2017 03:37 PM, Or Gerlitz wrote: >>> >>> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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. >> >> >> Squashing patches for the sake of squashing patches is inherently broken and >> bad. So please calm down and stop this mangling of other peoples' patches. >> >> If you want an alternative, put up a proposal and look at it for comparison >> somewhere. > > Hey Jes, > > It is not just about squashing patches, I am working on a series of > patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc > altogether, it will come out cleaner as it will remove all ethernet > sriov/eswitch VF representors and eswitch tc offloads stuff with one > kconfig flag, and yet preserve standard QoS functionality from en_tc. Saeed, I realize it is not just about squashing patches, however doing that to someone else's patches is just broken. The Linux kernel way is to build on top of patches, if they are valid, rather than throwing them all away and doing it from scratch again bottom up. If there was something actually wrong with my patches, and I would love to understand if that is the case, since I don't know 1/100th of the hardware details that you know, then please share those details. > BTW today you can just remove eswitch from driver and non sriov > configuration will perfectly work with no issues. > Even multi PF configuration will also work, but without l2 mac table, > which means PFs can only see packets with their own static (permanent) > mac addresses, user configured macs will not work on Multi PF > configuration. It sounds like this shakes up things a little and we will have things moved to where they actually belong in the hierarchy so that will be a good thing in the end :) > For that i will take the l2 table (ConnectX PF mac table) logic out of > eswitch as it is not really an eswitch logic, and move it to core > driver to allow Multi PF configuration to work without eswitch. Sounds good. > I will post some patches for you to review by end of week. Could we please start seeing this stuff happen in a public git tree so it is possible to follow and contribute to the development? It is very frustrating having to wait for things to appear and and not knowing whether a patch is integrated or needs to be revised when you have things building on top of it. Jes
On Wed, Jun 7, 2017 at 12:46 AM, Jes Sorensen <jsorensen@fb.com> wrote: > On 06/05/2017 05:53 PM, Saeed Mahameed wrote: >> >> On Mon, Jun 5, 2017 at 11:51 PM, Jes Sorensen <jsorensen@fb.com> wrote: >>> >>> On 06/03/2017 03:37 PM, Or Gerlitz wrote: >>>> >>>> >>>> On Fri, Jun 2, 2017 at 11:22 PM, Jes Sorensen <jsorensen@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.sorensen@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.sorensen@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. >>> >>> >>> >>> Squashing patches for the sake of squashing patches is inherently broken >>> and >>> bad. So please calm down and stop this mangling of other peoples' >>> patches. >>> >>> If you want an alternative, put up a proposal and look at it for >>> comparison >>> somewhere. >> >> >> Hey Jes, >> >> It is not just about squashing patches, I am working on a series of >> patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc >> altogether, it will come out cleaner as it will remove all ethernet >> sriov/eswitch VF representors and eswitch tc offloads stuff with one >> kconfig flag, and yet preserve standard QoS functionality from en_tc. > > > Saeed, > > I realize it is not just about squashing patches, however doing that to > someone else's patches is just broken. The Linux kernel way is to build on > top of patches, if they are valid, rather than throwing them all away and > doing it from scratch again bottom up. If there was something actually wrong > with my patches, and I would love to understand if that is the case, since I > don't know 1/100th of the hardware details that you know, then please share > those details. Hey Jes, Sorry for the inconvenience, I am working on a very similar patches, even before you posted yours. Your patches are fine, but as i said before, removing eswitch as is will introduce a small regression in Multi-PF configuration. the issue is that lately we are having tons of discussions exactly about this and how to do the driver breakdown that makes everyone happy, so things are moving relatively slow, but my work on eswitch is converging. > >> BTW today you can just remove eswitch from driver and non sriov >> configuration will perfectly work with no issues. >> Even multi PF configuration will also work, but without l2 mac table, >> which means PFs can only see packets with their own static (permanent) >> mac addresses, user configured macs will not work on Multi PF >> configuration. > > > It sounds like this shakes up things a little and we will have things moved > to where they actually belong in the hierarchy so that will be a good thing > in the end :) > >> For that i will take the l2 table (ConnectX PF mac table) logic out of >> eswitch as it is not really an eswitch logic, and move it to core >> driver to allow Multi PF configuration to work without eswitch. > > > Sounds good. > >> I will post some patches for you to review by end of week. > > > Could we please start seeing this stuff happen in a public git tree so it is > possible to follow and contribute to the development? It is very frustrating > having to wait for things to appear and and not knowing whether a patch is > integrated or needs to be revised when you have things building on top of > it. > Sure, I will post some patches later today. I believe they will be fully ready by for submission by End of week. Again sorry about this. > Jes
On 06/07/2017 12:06 AM, Saeed Mahameed wrote: > On Wed, Jun 7, 2017 at 12:46 AM, Jes Sorensen <jsorensen@fb.com> wrote: >>> Hey Jes, >>> >>> It is not just about squashing patches, I am working on a series of >>> patches to allow compiling out eswitch/eswitch_offloads/en_rep.c/en_tc >>> altogether, it will come out cleaner as it will remove all ethernet >>> sriov/eswitch VF representors and eswitch tc offloads stuff with one >>> kconfig flag, and yet preserve standard QoS functionality from en_tc. >> >> >> Saeed, >> >> I realize it is not just about squashing patches, however doing that to >> someone else's patches is just broken. The Linux kernel way is to build on >> top of patches, if they are valid, rather than throwing them all away and >> doing it from scratch again bottom up. If there was something actually wrong >> with my patches, and I would love to understand if that is the case, since I >> don't know 1/100th of the hardware details that you know, then please share >> those details. > > Hey Jes, > > Sorry for the inconvenience, I am working on a very similar patches, > even before you posted yours. > Your patches are fine, but as i said before, removing eswitch as is > will introduce a small regression in Multi-PF configuration. > > the issue is that lately we are having tons of discussions exactly > about this and how to do the driver breakdown > that makes everyone happy, so things are moving relatively slow, but > my work on eswitch is converging. Gotcha. I deliberately didn't disable eswitch itself in my patch, but only the offloading functionality, because of the old discussion mentioning that the eswitch needing to be initialized. >> Sounds good. >> >>> I will post some patches for you to review by end of week. >> >> >> Could we please start seeing this stuff happen in a public git tree so it is >> possible to follow and contribute to the development? It is very frustrating >> having to wait for things to appear and and not knowing whether a patch is >> integrated or needs to be revised when you have things building on top of >> it. > > Sure, I will post some patches later today. > I believe they will be fully ready by for submission by End of week. > Again sorry about this. Awesome! Jes
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile index 9e64461..5967b97 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile +++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile @@ -5,11 +5,13 @@ mlx5_core-y := main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \ mad.o transobj.o vport.o sriov.o fs_cmd.o fs_core.o \ fs_counters.o rl.o lag.o dev.o -mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.o \ +mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o \ en_main.o en_common.o en_fs.o en_ethtool.o en_tx.o \ en_rx.o en_rx_am.o en_txrx.o en_clock.o vxlan.o \ en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) += en_dcbnl.o +mlx5_core-$(CONFIG_MLX5_EN_ESWITCH_OFFLOADS) += en_eswitch_offloads.o + mlx5_core-$(CONFIG_MLX5_CORE_IPOIB) += ipoib.o diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c index 638b84f..320d1c5 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c @@ -43,7 +43,6 @@ enum { FDB_SLOW_PATH }; -#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS struct mlx5_flow_handle * mlx5_eswitch_add_offloaded_rule(struct mlx5_eswitch *esw, struct mlx5_flow_spec *spec, @@ -426,7 +425,6 @@ static int esw_add_fdb_miss_rule(struct mlx5_eswitch *esw) kvfree(spec); return err; } -#endif #define ESW_OFFLOADS_NUM_GROUPS 4 @@ -477,7 +475,6 @@ static void esw_destroy_offloads_fast_fdb_table(struct mlx5_eswitch *esw) #define MAX_PF_SQ 256 -#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS static int esw_create_offloads_fdb_tables(struct mlx5_eswitch *esw, int nvports) { int inlen = MLX5_ST_SZ_BYTES(create_flow_group_in); @@ -737,7 +734,6 @@ static int esw_offloads_start(struct mlx5_eswitch *esw) return err; } -#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS int esw_offloads_init(struct mlx5_eswitch *esw, int nvports) { struct mlx5_eswitch_rep *rep; @@ -796,7 +792,6 @@ int esw_offloads_init(struct mlx5_eswitch *esw, int nvports) return err; } -#endif static int esw_offloads_stop(struct mlx5_eswitch *esw) { @@ -819,7 +814,6 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw) return err; } -#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports) { struct mlx5_eswitch_rep *rep; @@ -836,7 +830,6 @@ void esw_offloads_cleanup(struct mlx5_eswitch *esw, int nvports) esw_destroy_offloads_table(esw); esw_destroy_offloads_fdb_tables(esw); } -#endif static int esw_mode_from_devlink(u16 mode, u16 *mlx5_mode) { @@ -1124,7 +1117,6 @@ int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink, u8 *encap) return 0; } -#ifdef CONFIG_MLX5_EN_ESWITCH_OFFLOADS void mlx5_eswitch_register_vport_rep(struct mlx5_eswitch *esw, int vport_index, struct mlx5_eswitch_rep *__rep) @@ -1169,4 +1161,3 @@ struct net_device *mlx5_eswitch_get_uplink_netdev(struct mlx5_eswitch *esw) rep = &offloads->vport_reps[UPLINK_REP_INDEX]; return rep->netdev; } -#endif
This gets rid of the temporary #ifdef spaghetti and allows the code to compile without offload support enabled. Signed-off-by: Jes Sorensen <jsorensen@fb.com> --- drivers/net/ethernet/mellanox/mlx5/core/Makefile | 4 +++- drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 9 --------- 2 files changed, 3 insertions(+), 10 deletions(-)