On 13 January 2015 at 07:03, Ola Liljedahl <[email protected]> wrote:
> 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. > > I agree with Ola D, and I think the sentiment is echoed by a number of others, that tighter patches are better, apart from that being the learned wisdom across git usage in the world IMHO. To take a potential case the issue on Friday, if the synchronizers patches had been split by API, each adding a group of tests to the file it is possible that a trivial bisect would have allowed me to accept a large percentage of the patchs and left the smoking gun pointing at a specific section of code. As a non expert in that code that is the best I am going to do, I don't have the time to dig deeper and I suspect other external users of ODP will be in the same boat. We keep bumping along on this topic and I think it is time to clearly state that *ODP expects small logical patches that build to the goal of making a complex change* > > > > 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 > -- *Mike Holmes* Linaro Sr Technical Manager LNG - ODP
_______________________________________________ lng-odp mailing list [email protected] http://lists.linaro.org/mailman/listinfo/lng-odp
