Refactoring Congestion Control (take 2)

[Rui Paulo]

Hi.
I've changed my patch a bit more:

1) I've received the suggestion of renaming "ctrl" to "ctl" like the
userland utilites, and so I did it.

2) A new sysctl tree:
$ sysctl net.inet.tcp.congctl
net.inet.tcp.congctl.available = reno newreno
net.inet.tcp.congctl.selected = newreno

3) I've deleted net.inet.tcp.newreno. What's the policy on delete
sysctls nodes? I would love to be able to kill this one, but I'm not
sure I can.

4) I'm facing a problem with our sysctl API.

# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = newreno
# sysctl -w net.inet.tcp.congctl.selected=reno
sysctl: net.inet.tcp.congctl.selected: sysctl() failed with Cannot
allocate memory
# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = reno

Any idea on what might be the cause?

Any other comments?

Regards,

-- Rui Paulo

[YAMAMOTO Takashi]

Post by Rui Paulo
Any other comments?

i think it's better to copy tcp_congctl_global to a member in tcpcb
so that it's somewhat static for a given connection.
switching the sysctl knob correctly when it affects existing connections
is a locking nightmare.

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi

Post by Rui Paulo
Any other comments?

i think it's better to copy tcp_congctl_global to a member in tcpcb
so that it's somewhat static for a given connection.
switching the sysctl knob correctly when it affects existing
connections
is a locking nightmare.

Maybe something like:

[YAMAMOTO Takashi]

if (maxburst < 0)
printf("tcp_output: maxburst exceeded by %d\n", -maxburst);
#endif
- if (sendalot && (!tcp_do_newreno || --maxburst))
+ if (sendalot && (tp->t_congctl == &tcp_reno_ctl || --maxburst))
goto again;
return (0);
}

isn't it better to make this another callback?

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi

if (maxburst < 0)
printf("tcp_output: maxburst exceeded by %d\n", -maxburst);
#endif
- if (sendalot && (!tcp_do_newreno || --maxburst))
+ if (sendalot && (tp->t_congctl == &tcp_reno_ctl || --maxburst))
goto again;
return (0);
}

isn't it better to make this another callback?

Hmm. It may be, thanks.

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by Rui Paulo
4) I'm facing a problem with our sysctl API.
# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = newreno
# sysctl -w net.inet.tcp.congctl.selected=reno
sysctl: net.inet.tcp.congctl.selected: sysctl() failed with Cannot
allocate memory
# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = reno

I still have this problem. Any ideas?

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo

Post by Rui Paulo
4) I'm facing a problem with our sysctl API.
# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = newreno
# sysctl -w net.inet.tcp.congctl.selected=reno
sysctl: net.inet.tcp.congctl.selected: sysctl() failed with Cannot
allocate memory
# sysctl net.inet.tcp.congctl.selected
net.inet.tcp.congctl.selected = reno

I still have this problem. Any ideas?
--
Rui Paulo

sysctl_createv(clog, 0, NULL, NULL,
CTLFLAG_PERMANENT|CTLFLAG_READWRITE,
CTLTYPE_STRING, "selected",
SYSCTL_DESCR("Selected Congestion Control Mechanism"),
sysctl_tcp_congctl, 0, &tcp_congctl_global_name, 0,
CTL_NET, pf, IPPROTO_TCP, congctl_node,
CTL_CREATE, CTL_EOL);

should be:

sysctl_tcp_congctl, 0, NULL, TCPCC_MAXLEN,

yes, our sysctl is too cryptic.

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi
sysctl_createv(clog, 0, NULL, NULL,
CTLFLAG_PERMANENT|CTLFLAG_READWRITE,
CTLTYPE_STRING, "selected",
SYSCTL_DESCR("Selected Congestion Control Mechanism"),
sysctl_tcp_congctl, 0, &tcp_congctl_global_name, 0,
CTL_NET, pf, IPPROTO_TCP, congctl_node,
CTL_CREATE, CTL_EOL);
sysctl_tcp_congctl, 0, NULL, TCPCC_MAXLEN,
yes, our sysctl is too cryptic.

Thanks a lot for spotting this, it now works perfectly!

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

I'm going to commit:
http://www.netbsd.org/~rpaulo/tcp_congctl.diff

in the upcoming week if there are no objections.
The setsockopt() handling is not done yet, but I'm planing to add
that feature later.

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo
http://www.netbsd.org/~rpaulo/tcp_congctl.diff
in the upcoming week if there are no objections.
The setsockopt() handling is not done yet, but I'm planing to add
that feature later.
--
Rui Paulo

"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

btw, where sack will be in the whole picture, eventually?

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi
"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

Well, "inflation" is probably not the best word, but I don't see why
we want to unify them. What do you have in mind?

Post by YAMAMOTO Takashi
btw, where sack will be in the whole picture, eventually?

I don't know yet. SACK, like ECN, can work with Reno, NewReno and
other congestion control algorithms. At least, I'm sure we don't want:
congctl.available = reno newreno reno-sack newreno-sack, etc.

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo

Post by YAMAMOTO Takashi
"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

Well, "inflation" is probably not the best word, but I don't see why
we want to unify them. What do you have in mind?

because both of them are called sequentially in most cases
(unless rcvacktoomuch, for which we can't do much anyway),
it isn't clear for me what's the benefit to have two callbacks
rather than one.

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi

Post by Rui Paulo

Post by YAMAMOTO Takashi
"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

Well, "inflation" is probably not the best word, but I don't see why
we want to unify them. What do you have in mind?

because both of them are called sequentially in most cases
(unless rcvacktoomuch, for which we can't do much anyway),
it isn't clear for me what's the benefit to have two callbacks
rather than one.

Right now, I don't see a way to have just one callback because of SACK.
We would have to call tcp_sack_newack() in every other
tcp_xxxx_cwnd_inflation().
The point here is to introduce HSTCP and Westwood+ after these changes.

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by Rui Paulo

Post by YAMAMOTO Takashi

Post by Rui Paulo

Post by YAMAMOTO Takashi
"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

Well, "inflation" is probably not the best word, but I don't see why
we want to unify them. What do you have in mind?

because both of them are called sequentially in most cases
(unless rcvacktoomuch, for which we can't do much anyway),
it isn't clear for me what's the benefit to have two callbacks
rather than one.

Right now, I don't see a way to have just one callback because of SACK.
We would have to call tcp_sack_newack() in every other
tcp_xxxx_cwnd_inflation().

if you have sack, whatever xxxx is, tcp_xxxx_cwnd_inflation isn't
needed to be called?
(i'm not sure about the semantics of cwnd_inflation.)

Oh yes, you are right. I'll remove the new_data_acked and
cwnd_inflation callbacks and add a new one called 'newack' (the name
change is to use what we already have: tcp_reno_newack,
tcp_sack_newack, etc.).

Post by Rui Paulo
The point here is to introduce HSTCP and Westwood+ after these changes.

these callbacks should be separated for them, you mean?

No, I was just saying what I would do next ;-)

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo

Post by YAMAMOTO Takashi

Post by Rui Paulo

Post by YAMAMOTO Takashi
"cwnd_inflation" sounds weird to me, given that what it does is
ack handling for fast recovery. otoh, "new_data_acked" inflates cwnd.
isn't it better to unify these two callbacks?

Well, "inflation" is probably not the best word, but I don't see why
we want to unify them. What do you have in mind?

because both of them are called sequentially in most cases
(unless rcvacktoomuch, for which we can't do much anyway),
it isn't clear for me what's the benefit to have two callbacks
rather than one.

Right now, I don't see a way to have just one callback because of SACK.
We would have to call tcp_sack_newack() in every other
tcp_xxxx_cwnd_inflation().

if you have sack, whatever xxxx is, tcp_xxxx_cwnd_inflation isn't
needed to be called?
(i'm not sure about the semantics of cwnd_inflation.)

Post by Rui Paulo
The point here is to introduce HSTCP and Westwood+ after these changes.

these callbacks should be separated for them, you mean?

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Thinking again... how can we check for (tp->t_dupacks >
tcprexmtthresh) (t_dupacks is changed when we recieve new data) ?

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by Rui Paulo
Thinking again... how can we check for (tp->t_dupacks >
tcprexmtthresh) (t_dupacks is changed when we recieve new data) ?

Heck, forget this question.

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Sorry but I guess I managed to misread the functions in question.

So, resuming the discussion:
1) Wouldn't we be breaking the standard, and maybe, adding more
processing in the case of SEQ_GT th->th_ack, tp->snd_max ?
2) I think it's safer to stop/restart the retransmit timers before
moving on to the adjusting the congestion window.
3) This is probably a matter of taste, but I prefer it this way.
Maybe I'm wrong, but I think it doesn't involve any performance
drawback this way.

What do you think?

--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo
Sorry but I guess I managed to misread the functions in question.
1) Wouldn't we be breaking the standard, and maybe, adding more
processing in the case of SEQ_GT th->th_ack, tp->snd_max ?

SEQ_GT(th->th_ack, tp->snd_max) mean broken peer or such, doesn't it?
is it stated in a standard? i couldn't find it.

Post by Rui Paulo
2) I think it's safer to stop/restart the retransmit timers before
moving on to the adjusting the congestion window.

i don't think it makes differences as we are at splsoftnet,
which blocks timers.

Post by Rui Paulo
3) This is probably a matter of taste, but I prefer it this way.
Maybe I'm wrong, but I think it doesn't involve any performance
drawback this way.
What do you think?

i don't have a strong preference, so if you prefer it for some reason,
i have no trouble with it.

after all, my main concern was about names of callbacks, rather than
number of callbacks. :) i'd suggest:
"cwnd_inflation" -> "fast_retransmit_newack"
"new_data_acked" -> "newack"

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Rui Paulo]

Post by YAMAMOTO Takashi

Post by Rui Paulo
Sorry but I guess I managed to misread the functions in question.
1) Wouldn't we be breaking the standard, and maybe, adding more
processing in the case of SEQ_GT th->th_ack, tp->snd_max ?

SEQ_GT(th->th_ack, tp->snd_max) mean broken peer or such, doesn't it?
is it stated in a standard? i couldn't find it.

Yes, I think so. I couldn't find it either.

Post by YAMAMOTO Takashi

Post by Rui Paulo
2) I think it's safer to stop/restart the retransmit timers before
moving on to the adjusting the congestion window.

i don't think it makes differences as we are at splsoftnet,
which blocks timers.

Good point.

Post by YAMAMOTO Takashi

Post by Rui Paulo
3) This is probably a matter of taste, but I prefer it this way.
Maybe I'm wrong, but I think it doesn't involve any performance
drawback this way.
What do you think?

i don't have a strong preference, so if you prefer it for some reason,
i have no trouble with it.
after all, my main concern was about names of callbacks, rather than
number of callbacks. :)

Oh, sorry about it then, I thought you meant to combine the two
together.

Post by YAMAMOTO Takashi
"cwnd_inflation" -> "fast_retransmit_newack"
"new_data_acked" -> "newack"

Will do that.

Thanks,
--
Rui Paulo





--
Rui Paulo







--
Rui Paulo



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[YAMAMOTO Takashi]

Post by Rui Paulo

Post by YAMAMOTO Takashi

Post by Rui Paulo
Sorry but I guess I managed to misread the functions in question.
1) Wouldn't we be breaking the standard, and maybe, adding more
processing in the case of SEQ_GT th->th_ack, tp->snd_max ?

SEQ_GT(th->th_ack, tp->snd_max) mean broken peer or such, doesn't it?
is it stated in a standard? i couldn't find it.

Yes, I think so. I couldn't find it either.

then, in that case, we can just goto dropafterack without calling
cwnd_inflate, i think.

Post by Rui Paulo

Post by YAMAMOTO Takashi

Post by Rui Paulo
3) This is probably a matter of taste, but I prefer it this way.
Maybe I'm wrong, but I think it doesn't involve any performance
drawback this way.
What do you think?

i don't have a strong preference, so if you prefer it for some reason,
i have no trouble with it.
after all, my main concern was about names of callbacks, rather than
number of callbacks. :)

Oh, sorry about it then, I thought you meant to combine the two
together.

well, i meant to combine them, yes.
but it wasn't my main concern, so i don't insist on that.

YAMAMOTO Takashi

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de