On Mon, Aug 28, 2006 at 12:18:33PM +1200, Ian McDonald
wrote:
> It does raise the question though is whether the dccp
mailing list is
> worth keeping separate? Thoughts?
I'm registered on dccp only because netdev is too noisy =D
I personally think
DCCP should gain more momentum before entering netdev.
Also, I still need to
understand if stuff can go directly to dave or through
arnaldo, especially when
arnaldo gets killed with work.
> You need a description of each patch and signed off
line in each
> patch. They can't be accepted in this form.
Alternatively they could
> be resubmitted.
The proper email, patch and log description was sent a while
back in the DCCP
mailing list. About resubmitting, I hate insisting. If
people drop mail, there
probably is a reason. It's one of my stubburn principles
=P.
> Please state whether the target is 2.6.18 (bug fixes
only) or 2.6.19
> (enhancements)
Some are bug fixes, but I'd send it all to .19. It's a
milestone patchset:
enable DCCP to do gbit [with ccid2 at least].
> Please state any dependencies between patches.
just apply them sequentially. Most are independent. I
specifically put them in
an order "most likeley to be accepted -> most
likeley to be rejected". If you
notice, the early patches are bug fixes. The latest patches
are stuff like
"lets add profilin support".
> In patch 01_ackvec_opt:
> -int dccp_ackvec_parse(struct sock *sk, const struct
sk_buff *skb,
> +u64 dccp_ackvec_parse(struct sock *sk, u64 ackno,
> const u8 opt, const u8 *value,
const u8 len)
>
> This becomes a one line function. This is only used in
one place that I can
> see so this should go and that code should go there...
Also there is some
yea I agree. I'll do it once I see that the first patch
set starts getting
processed.
> weird shit going on as this is also defined as inline
with return -1 in
> ackvec.h. This needs fixing as well.
I dunno wtf that is =D That's some old debris that needs
to get killed.
> In patch 05_ccid2_seq_alloc I don't get this code:
> If you are allocating an array of structures in effect
you shouldn't
> need to set next/prev pointers as they are allocated
contiguously. If
I am allocating records in a double linked list. They will
be moved around in
time, so I need the pointers to keep them in the order I
wish.
> you are allocating groups of arrays, which I suspect
you are, I still
> think the design is a bit ugly and wastes memory.
I agree it's ugly, but I'm not a guru. I wouldn't know
how to do it otherwise
[without spending time on it]. I maintain my own free-list
of structures and
reuse them as necessary. I don't think it wastes that much
memory. I do one
malloc for a bunch of structures. Then I shove that pointer
into a "recycle
bin" so I can free it at the end. What would the
alternative be? One malloc
per structure? That would probably waste more memory and
CPU.
> In 06_ccid2_ssthresh you don't justify why ssthresh
can be infinite to
> start off with. And if it is allowed then I don't
think you should do
I think by definition you exist slow start when you hit
packet loss. I believe
this maps to an infinite sshtresh.
> this by just picking a random high number. Change it to
something like
> ~0
I agree. [The number is not random; the original choice I
made was too low so I
had to repeat the digit.]
> In 07_ccid2_send_poll - changing to 1 msec poll is not
nice. I now you
> said this should be dequeued. I've just added tx
queuing to 2.6.19
> tree now so you can do this. Up to Dave/Arnaldo if this
is OK as a
> short term solution.
I personally think CCID should call-back to DCCP, "yo
dude, you can send". What
polling frequency should I put there? Intuitively it should
be
time_when_next_ack_arrives - now. But when is that time?
RTT/cwnd?
> In 08_ccid2_cwnd:
> I think that should be a ccid2_pr_debug not a
dccp_pr_debug
agree.
> in 10_ccid2_change:
> in ccid2_change_srtt why do you do the test there
because you are
> going to take twice as many instructions if you are
going to alter. Is
> this to minimise sets?
yea to lower profiling information. Many times the srtt
doesn't change. [Well
in lab environments.]
> in 11_ccid2_profile:
> in ccid2_profile_time the code there is ugly. I am
guessing you are
> assuming 32 bit intel vs 64 bit intel. The world
doesn't revolve
> around intel. If you need something architecture
specific that should
> be put into the arch subtree not here.
I don't expect this to be merged. Using a kprobe in the
"change_X" functions
does the same. I discovered kprobes later. The refactoring
to the change_X
methods chould be kept though.
> in ccid2_hc_tx_init why are you using 6000? What is the
significant.
> Make it a constant defined somewhere.
Random number. Again, the profiling was a hack to
"see" CCID2. It turned out
to be really useful when testing the high speed stuff
[tcp_cong_whateveritscalled].
Dude, thanks for ur time in this. I'll address the issues
I agreed on in this
mail when I see arnaldo, or anyone, playing and hopefully
commiting these
patches.
-
To unsubscribe from this list: send the line
"unsubscribe dccp" in
the body of a message to majordomo vger.kernel.org
More majordomo info at http://vge
r.kernel.org/majordomo-info.html
|