| > This patch addresses the following situation:
| > * timestamps are recorded on the listening socket;
|
| I noticed this, I think it is unaceptable to do that, therefore the best
| thing is to combine the two patches, so as not to introduce a problem
| that is fixed in the following patch.
OK, so we have at least agreement that something needs to be done. Sorry I don't understand the
above remark: do you refer to the hunk which fixed a pointer assignment (tse = ...) and slipped
into the wrong patch?
I am happy to combine the two patches if that is what you are asking. I had kept the next patch
("Support inserting options during the 3-way handshake") separate since it is mainly concerned
with inserting options, which is called by feature-negotiation (see below for related issue).
| > This patch therefore
| > * makes memory use for timestamps dynamic (to use less when no timestamps are present);
| > * separates `handshake timestamps' from `established timestamps';
|
| I didn't understood this one. Care to further explain?
Yes - awkward sentence. What I meant (and that becomes only obvious further down in the patch set) is:
1) First point: if no timestamp is present then there is no memory (apart from a pointer) associated
with this. I was concerned that otherwise the request_socks get too big - which is exactly your
comment from below. This is not the end of the story - there is also a list_head on request_socks
to store the feature-negotiation list. Here I had the same concerns: initially I wanted to use a
table for all features, and that would have been a fat use of memory (9 features x 2 locations
(local/remote) x 64bit (maximum option size) gives 1152 bytes - which I found utterly inacceptable
to put on a request_sock. Instead, the feature negotiation uses a list which is allocated at most
once (even when the Request is retransmitted, it is not allocated again).
And for the timestamps I thought it would be good practice to do the same - if no timestamps are
present during the handshake, don't allocate memory for them.
2) Separating `handshake timestamps' from `established timestamps': I don't know where my mind was,
this just means that timestamps can appear during the handshake (which needs special considerations
on the server) and when the connection is fully established (then it uses the sk fields like before).
| Anyway, I think that adding yet another allocation in a connection lifetime is not good.
Please see below - I can see your point, but other issues are involved also.
| One of the most pressing things for me after merging all the patches
| that are pending (THANK YOU! :-) ) is to work on DCCP memory consuption
| and accounting, the code has to be made more robust :-\
The coming feature negotiation patch set removes lots of kmallocing and kmemduping which is frequent in
the current feature negotiation code. I don't know if that is what you mean, but I also observed problems
with the cache when I did the tests.
| I think that we should just add dreq_{time,echo} to dccp_request_sock
| and keep dccp_sock as is.
|
<snip>
< ...
| Humm, these minisocks are getting fat... another thing for my TODO list,
That is my concern, too. So what shall we do: add two fields which may not be used most of the
time (at the moment only to support an erratum to CCID3), or keep the memory allocation.
| request_sock::ts_recent seems to be used only by the TCP machinery, ripe
| for the picking....
Is there any use of ts_recent for CCID2 - in an earlier email you pointed out that CCID2
could do timestamping, and that would be a place to use.
|
| Anyway, I'll move along the patch queue looking for more easily
| cherrypickable patches.
Please don't at the moment consider the feature negotiation patches for cherry picking -
there are more to come; I split them into batches to make reviewing easier (and your input
is appreciated).
-
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
