Em Fri, Nov 23, 2007 at 10:49:29AM +0000, Gerrit Renker escreveu:
> | ------------------------------------------------------
> | The patch set migrates TFRC TX history to a singly-linked list.
> |
> | The details are:
> | * use of a consistent naming scheme (all TFRC functions now begin with `tfrc_');
> | * allocation and cleanup are taken care of internally;
> | * provision of a lookup function, which is used by the CCID TX infrastructure
> | to determine the time a packet was sent (in turn used for RTT sampling);
> | * integration of the new interface with the present use in CCID3.
> | ------------------------------------------------------
> |
> | Simplifications I did:
> |
> | . removing the tfrc_tx_hist_head that had a pointer to the list head and
> | another for the slabcache.
> | . No need for creating a slabcache for each CCID that wants to use the TFRC
> | tx history routines, create a single slabcache when the dccp_tfrc_lib module
> | init routine is called.
> |
> These two above points are a good job and are useful. Well done. However, you have
> added other changes which are not in the above list, where I would like to make two suggestions.
>
> Also did you test this code? The patch on which this is based has been
> subjected to some extensive regression-testing. I can't tell if it will
> perform as intended.
I performed testing, not exhaustive as the code should be equivalent to
what you had done, or can you find, from code inspection, something
suspicious?
> | --- a/net/dccp/ccids/ccid3.c
> | +++ b/net/dccp/ccids/ccid3.c
> | static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
> | {
> | struct ccid3_hc_tx_sock *hctx = ccid3_hc_tx_sk(sk);
> | struct ccid3_options_received *opt_recv;
> | + struct tfrc_tx_hist_entry *packet;
> | ktime_t now;
> | unsigned long t_nfb;
> | u32 pinv, r_sample;
> | @@ -425,16 +414,19 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
> | switch (hctx->ccid3hctx_state) {
> | case TFRC_SSTATE_NO_FBACK:
> | case TFRC_SSTATE_FBACK:
> | + /* estimate RTT from history if ACK number is valid */
> | + packet = tfrc_tx_hist_find_entry(hctx->ccid3hctx_hist,
> | + DCCP_SKB_CB(skb)->dccpd_ack_seq);
> | + if (packet == NULL) {
> | + DCCP_WARN("%s(%p): %s with bogus ACK-%llu\n", dccp_role(sk), sk,
> | + dccp_packet_name(DCCP_SKB_CB(skb)->dccpd_type),
> | + (unsigned long long)DCCP_SKB_CB(skb)->dccpd_ack_seq);
> | return;
> | }
> | + /*
> | + * Garbage-collect older (irrelevant) entries
> | + */
> | + tfrc_tx_hist_purge(&packet->next);
> |
> | /* Update receive rate in units of 64 * bytes/second */
> | hctx->ccid3hctx_x_recv = opt_recv->ccid3or_receive_rate;
> <snip>
> | /*
> | * Calculate new RTT sample and update moving average
> | */
> | + r_sample = dccp_sample_rtt(sk, ktime_us_delta(now, packet->stamp));
> | hctx->ccid3hctx_rtt = tfrc_ewma(hctx->ccid3hctx_rtt, r_sample, 9);
> |
> Sorry but I don't like this at all.
>
> All the low-level details of the packet_history interface are now again
> integrated into ccid3.c, so that ccid3.c ends up doing the job that
> packet_history can just as well do:
>
> * first find the entry
> * then purge all older entries
> * then look into the packet entry to read the timestamp
>
> CCID3 (and for that matter CCID4 also) use the TX history only for a single purpose - to look
> up the send time. If there was another method, the entire TX history could go.
>
> The main intention of this patch set has been to separate more clearly between ccid3.c,
> loss_interval.c, and packet_history.c. If RTT measurement is done in the above way, then
> we don't gain much from the new patches.
Well we gain the fact that it is now uses a single linked list, that is
what I tried to do in simplifying this patch.
> And other modules using tfrc_lib, such as CCID4,
> will have to perform the same manual steps all over again (i.e. code duplication).
>
> Can you therefore please consider to keep the function tfrc_hist_when() from the original patch
> (or using a similar name).
>
> That function only exposes what CCID3 is interested in - the send time - and takes care of the
> low-level detail. With regard to your new interface, it would look like
>
> int tfrc_tx_hist_when(ktime_t *stamp, struct tfrc_tx_hist_head *head, u64 ackno)
> {
> struct tfrc_tx_hist_entry *entry = tfrc_tx_find_entry(head, ackno);
>
> if (entry != NULL) {
> *stamp = entry->stamp;
> tfrc_tx_hist_purge(&entry->next);
> return 1;
> }
> return 0;
> }
>
> One of the main problems in the TFRC/CCID3 code has been (and to some extent still is)
> that the code is so deeply intertwined that it is almost impossible to keep functionality separate.
I have no problem with this.
So we do:
1. convert the TX history to use a single linked list and use
just one slabcache.
2. hide the internal TX history implementation details, since
what the CCIDs are interested in is the time the packet
being acknowledged or the extreme case where it couldn't find it
in whatever data structure the TX history implementation uses.
> The comment below has the same background, but I suspect that you did
> this only as a temporary solution to make the module compile?
Yes, see below.
> | --- a/net/dccp/ccids/lib/loss_interval.c
> | +++ b/net/dccp/ccids/lib/loss_interval.c
> |
> | -static __init int dccp_li_init(void)
> | +int __init dccp_li_init(void)
>
> | -static __exit void dccp_li_exit(void)
> | +void dccp_li_exit(void)
> Here it is declared non-static so that packet_history can call it.
>
> | --- a/net/dccp/ccids/lib/packet_history.c
> | +++ b/net/dccp/ccids/lib/packet_history.c
>
> | +extern int __init dccp_li_init(void);
> | +extern void dccp_li_exit(void);
> | +
> | +static __init int packet_history_init(void)
> | +{
> | + if (dccp_li_init() != 0)
> | + goto out;
> <snip>
> | +static __exit void packet_history_exit(void)
> | +{
> | + if (tfrc_tx_hist != NULL) {
> | + kmem_cache_destroy(tfrc_tx_hist);
> | + tfrc_tx_hist = NULL;
> | + }
> | + dccp_li_exit();
> | +}
> The routine is called `packet_history_init()' and calls an initialisation routine
> in a different module (loss_interval.c). It is the same problem again
> that loss_interval and packet_history keep being intertwined.
Because both end up in the same kernel module, dccp_tfrc_lib.ko, so it
doesn't matter where we put the module initialization routine, as long
as it calls what each part of the lib wants to be called at init time.
> A fix is available a few patches down the line; but more than likely you
> already noticed it. The patch introduces a separate source file
> tfrc_module.c which calls the individual initialisation routines of the
> source files that make up dccp_tfrc_lib:
>
> tfrc_module_init()
> - calls packet_history_init()
> - calls loss_interval_init()
> - does internal stuff
>
> I think this is clearer, and it is extensible since then there is a dedicated
> file for dccp_tfrc_lib which can take up all the code which does not serve one
> of the particular purposes
> * TFRC fixpoint lookup arithmetic (tfrc_equation.c)
> * header includes for TFRC fixpoint arithmetic (tfrc.h)
> * RX history and TX history interface (packet_history.{ch})
> * book-keeping for loss intervals and computing loss event rates (loss_interval.{c,h})
Yes, this is clearer and noticed it, will get to that.
Its just that when simplifying the TX history by removing the need for a
per CCID cache since the size of the object is the same for all clients,
I needed to, at module init time, to create the slab cache, and to keep
the patch as small as possible, I decided to use one of the existing
files in net/dccp/ccids/lib/.
- Arnaldo
-
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
