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
