Russell Stuart wrote:
> On Thu, 2006-10-19 at 16:38 +0200, Patrick McHardy wrote:
>
>>I still think this patch shouldn't go in. There's no point in doing the
>>same thing twice, and I haven't heard a compelling argument why it has
>>to be done in a way that only helps qdiscs using rtabs while ignoring
>>statistics and estimators (I even provided a patch to show how to do
>>it without these limitations).
>
>
> As far as I can see one patch changes the way the
> kernel calculates packet lengths, and the other modifies
> RTAB. I can not see the significant overlap between the
> patches that you talk about.
The implementation may be different, but the intention and the
result is the same. I probably would mind less if it wouldn't
affect userspace compatibility, but we need to carry this stuff
for ever even if we add another implementation that covers all
qdiscs.
> As for why haven't got a new patch from me that addresses
> the "doing the same thing twice" issue - it is because I
> can see no such issue. I have asked you repeatedly to
> explain it further, but you have not done so.
What do I need to explain further? As I stated several times,
I would like to see a patch that handles all qdiscs. And it
should probably not have hardcoded ATM values, there is other
media that behaves similar.
> As for "providing a patch" - I believe at the time you
> called it something you were working on. As far as I
> know you still are working on it. Besides, even if you
> did intend me to take it further, I am not particularly
> interested in the packet length issue as it does not
> effect the real world performance of any of the qdiscs
> I use.
No, I'm not working on it currently, it was more meant for
demonstration purposes. If you're not interested in taking
the opinion of people working on the code into account thats
your problem.
>>Besides that:
>>
>>+static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, int pktlen)
>>+{
>>+ int slot = pktlen + rtab->rate.cell_align;
>>+ if (slot < 0)
>>+ slot = 0;
>>
>>Why would it go negative? A negative cell_align doesn't make sense I
>>guess.
>
>
> A negative cell align is possible, and in fact is typical.
> If slot ended up being less than 0 then the configuration
> parameters passed to "tc" would of been wrong - they could
> not of matched the actual traffic. The error can't be
> detected in "tc", but it can't be allowed to cause the
> kernel to index off the end of an array either.
I'm not sure I understand what you're saying here. The transmission
time gets _smaller_ by transmitting over ATM? Does this have anything
to do with the off-by-one during rate table calculation you or
Jesper noticed?
>>+ slot >>= rtab->rate.cell_log;
>>+ if (slot > 255)
>>+ return rtab->data[255] + 1;
>>
>>Whats the point of this? Is it just to keep htb giant statistics
>>working?
>
>
> Yes.
TBF and policers already make sure this can never happen, this is
what HTB should do as well. If it is also needed for CBQ it is
a bugfix and should be done seperately.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
