2016-03-08 07:49, Dumitrescu, Cristian: > We are working on implementing an alternative solution based on 2x 64-bit > multiplication, which is supported by CPUs and compilers for more than a > decade now. The 32-bit solution proposed by Stephen requires truncation with > some loss of precision, which can potentially lead to some corner cases which > are difficult to predict, therefore I am not feeling 100% confident with it. > The 32-bit arithmetic gave me a lot of headaches when developing QoS code, > therefore I am very cautious of it. > > I am not sure we are able to finalize implementation and testing for release > 16.4, therefore it would be fair to accept Stephen's solution for release > 16.4 and consider the new safer 2x 64-bit multiplication solution which does > not involve any loss of precision once it becomes available. > > Regarding Stephen's patches, I think there is a pending issue regarding the > legal side of the Copyright, which is attributed to Intel, although Stephen's > code is relicensed with BSD license by permission from the original code > author (which also submitted the code to Linux kernel under GPL). This was > already flagged. This is a legal issue and I do not feel comfortable with > ack-ing this patch until the legal resolution on this is crystal clear. > > I also think the new files called rte_reciprocal.[hc] implement an algorithm > that is very generic and totally independent of the QoS code, therefore it > should be placed into a different folder that is globally visible to other > libraries (librte_eal/common ?) just in case other usages for this algorithm > are identified in the future. I suggest we break the patch into two separate > patches submitted independently: one introducing the rte_reciprocal.[hc] > algorithm to librte_eal/common and the second containing just the > librte_sched changes, which are small. I am thinking ahead here: once we have > the 2x64-bit multiplication solution in place, we should not have > rte_reciprocal.[hc] hanging in librte_sched folder without being used here, > while it might be used by other parts of DPDK.
Let's keep the improvement as-is to test it in the first release candidate. We can move the code and/or fix the file header later. Series applied, thanks.
