On 13 January 2015 at 12:50, Ola Dahl <[email protected]> wrote: > Hi, > > as a user of ODP I would like to add that in addition to bisectability there > are other, rather important advantages of small patches (where each patch > contains a well-defined piece of functionality). > > It is our experience, as users of ODP (i.e. as developers of applications on > top of ODP), that when we update to newer ODP versions, more specifically > when we updated from ODP 0.4.0 to ODP 0.5.0, we saw that > > - it was very difficult to understand, from the git log, what had been > changed > > As a consequence, our porting time increased, and moreover - when we found a > bug, it was difficult to understand how to report it and how to make a > temporary workaround (so that we were not blocked in our development). > > I guess that when many things are changed in one go, the possibility of diff > (and hence what is shown in the git log) to figure out how to print a > readable and understandable text decreases. > > In short, the culture of using large patches increases our (and others I > presume) costs for working with ODP. ODP pre-1.0 is still an unstable project. Feature and API development in ODP prioritized over application development. Of course this should change for 1.x releases with incremental changes and maintained backwards compatibility as far as possible.
> > Best regards, > > Ola D > Enea > > > > > On Tue Jan 13 2015 at 11:53:29 AM Ciprian Barbu <[email protected]> > wrote: >> >> On Mon, Jan 12, 2015 at 9:56 PM, Bill Fischofer >> <[email protected]> wrote: >> > I don't think bisect was of any particular help or hindrance in this >> > case, >> > especially since the buffer patch as merged was fully bisected (thanks >> > to >> > Taras). The actual problem was in a related file that went in as part >> > of >> > classification rather than the buffer restructure. >> >> I hate to divagate, but if Taras did take care of the patches being >> bisectable then something went wrong. The bug reported was indeed >> caused by the addition of the tcp header struct which was part of the >> classifier work, but that's only half the story. When bisecting I >> noticed that the packet example changed behavior like this: >> 0d93470 api: buffer: change pool create --> lots of duplicated ICMP >> packets, not just one >> df8a283 api: packet: change layer offset/pointer API --> no ICMP >> packets duplicates >> ee8c83f linux-generic: packet: improve packet parsing --> packet >> example working back again, with the added TCP header/endianess caused >> problem; this is where the TCP problem was introduced, because the >> patch now parses TCP headers too >> >> The actual tcp header definition along with the byte order bug was >> introduced way back: >> 08796a4 helper: odph_tcp header description >> >> Sorry again for hijacking the thread, hope this makes clear my findings. >> >> /Ciprian >> >> > >> > v2 of the patch follows Shmulik's suggestion, however. >> > >> > On Mon, Jan 12, 2015 at 1:16 PM, Mike Holmes <[email protected]> >> > wrote: >> >> >> >> >> >> >> >> On 11 January 2015 at 11:58, Bill Fischofer <[email protected]> >> >> wrote: >> >>> >> >>> The patch wasn't intended as a bug fix, but rather is intended to be a >> >>> more refined implementation of the buffer allocator that can permit >> >>> better >> >>> performance scalability testing as we move into 2015. It does have >> >>> the >> >>> effect, however, of fixing a couple of reported bugs that were the >> >>> result of >> >>> the previous implementation. >> >>> >> >>> I can post a repackaged version of this as v2 if that will ease >> >>> getting >> >>> it reviewed for inclusion in ODP 0.8, however, it just means asking >> >>> reviewers to keep track of and apply multiple patches instead of one >> >>> patch. >> >> >> >> >> >> I think it is worth it, as Ciprian found out with the big buffer patch >> >> we >> >> had impacted our ability to use bisect to narrow down an issue. >> >> >> >>> >> >>> >> >>> >> >>> >> >>> On Sun, Jan 11, 2015 at 1:56 AM, Shmulik Ladkani >> >>> <[email protected]> wrote: >> >>>> >> >>>> On Sat, 10 Jan 2015 20:51:28 -0600 Bill Fischofer >> >>>> <[email protected]> wrote: >> >>>> > Because in the presence of local buffer caching it is not clear >> >>>> > whether >> >>>> > lockless or lock-based allocation will scale better, this patch >> >>>> > adds >> >>>> > compile-time support for selecting which sychronization mechanism >> >>>> > to >> >>>> > use. >> >>>> > By default lockless allocation is used. To enable lock-based >> >>>> > allocation >> >>>> > change the USE_BUFFER_POOL_LOCKS define to 1. >> >>>> >> >>>> Thanks. >> >>>> >> >>>> May I suggest splitting this as a patch set? >> >>>> Introducing USE_BUFFER_POOL_LOCKS seems independent of the actual bug >> >>>> fix. >> >>>> >> >>>> Regards, >> >>>> Shmulik >> >>> >> >>> >> >>> >> >>> _______________________________________________ >> >>> lng-odp mailing list >> >>> [email protected] >> >>> http://lists.linaro.org/mailman/listinfo/lng-odp >> >>> >> >> >> >> >> >> >> >> -- >> >> Mike Holmes >> >> Linaro Sr Technical Manager >> >> LNG - ODP >> > >> > >> > >> > _______________________________________________ >> > lng-odp mailing list >> > [email protected] >> > http://lists.linaro.org/mailman/listinfo/lng-odp >> > >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > _______________________________________________ > lng-odp mailing list > [email protected] > http://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
