[PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Ivo Calado]

First Patch on TFRC-SP. Does a copy from TFRC and adjust symbols name
with infix "_sp".
Also updates Kconfig and init/exit code. An #ifndef was added to headers that
have commom symbols with TFRC that were not changed, so they don't get
included twice.


Following the rule #8 in Documentation/SubmittingPatches the patch is
stored at http://embedded.ufcg.edu.br/~ivocalado/dccp/patches_tfrc_sp/tfrc_sp_receiver_00.patch

[PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Ivo Calado]

First Patch on TFRC-SP. Does a copy from TFRC and adjust symbols name
with infix "_sp".
Also updates Kconfig and init/exit code. An #ifndef was added to headers
that
have commom symbols with TFRC that were not changed, so they don't get
included twice.


Following the rule #8 in Documentation/SubmittingPatches the patch is
stored at 
http://embedded.ufcg.edu.br/~ivocalado/dccp/patches_tfrc_sp/tfrc_sp_receiver_01.patch

[PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Ivo Calado]

First Patch on TFRC-SP. Does a copy from TFRC and adjust symbols name
with infix "_sp".
Also updates Kconfig and init/exit code. An #ifndef was added to headers
that
have commom symbols with TFRC that were not changed, so they don't get
included twice.


Following the rule #8 in Documentation/SubmittingPatches the patch is
stored at
http://embedded.ufcg.edu.br/~ivocalado/dccp/patches_tfrc_sp/tfrc_sp_receiver_01.patch

Re: [PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Gerrit Renker]

Hi Ivo,

you have made a really good job of sticking to code conventions (see other
posting). There are a few things that needed tending to in the first patch.

(1) Version changes
=========It seems that you applied something like s/\*\*/*/g to the first instance of
the patch, in order to remove duplicate asterisks. This caused a problem:

--- tfrc_sp_receiver_01.patch   2009/09/12 08:37:12     1.1
+++ tfrc_sp_receiver_01.patch   2009/09/08 17:34:40
@@ -1001,8 +1001,8 @@ Index: dccp_tree_work4/net/dccp/ccids/li
 +
 +#endif
 +
-+extern int  tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry **headp, u64 seqno);
-+extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry **headp);
++extern int  tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry *headp, u64 seqno);
++extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry *headp);

I have reverted the bug, also to minimise the difference to the existing (non
TFRC-SP) files.


(2) Other changes that I edited out
=================(Other than whitespace changes.)

net/dccp/ccids/lib/loss_interval_sp.c
-------------------------------------
I replaced the following dead code
        if ((tfrc_lh_slab != NULL))
                return 0;

        if (tfrc_lh_slab != NULL) {
                kmem_cache_destroy(tfrc_lh_slab);
                tfrc_lh_slab = NULL;
        }
        return -ENOBUFS;
with 
        return tfrc_lh_slab = NULL ? -ENOBUFS : 0;

Also separated the conditions
+               if ((len <= 0) || 
+                   (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))) {
back into 
                if (len <= 0)
                        return false;

                if (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))
                        return false;

I removed the following unnecessary inclusion:
+#include <linux/random.h>


The following function pokes a hole in thei so far "abstract" data type;
the convention has been to access the internals of the struct only via
get-functions:

static inline struct tfrc_loss_interval
        *tfrc_lh_get_loss_interval(struct tfrc_loss_hist *lh, const u8 i)
{
        BUG_ON(i >= lh->counter);
        return lh->ring[LIH_INDEX(lh->counter - i - 1)];
}

(You use it in patch 3/5 to gain access to li_ccval and li_losses.
Better would be to have two separate accessor functions.)


net/dccp/ccids/lib/tfrc_equation_sp.c
-------------------------------------
This is a prime candidate for removal. After editing out the whitespace
differences, I found that it is 100% identical with tfrc_equation.c.

The result of this editing has been uploaded to
http://eden-feed.erg.abdn.ac.uk/tfrc_sp_receiver_01.patch

Re: [PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Ivo Calado]

Hi Gerrit and all.
Thank you for your fast reply. My comments follow below

On Sat, Sep 12, 2009 at 15:32, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Hi Ivo,
>
> you have made a really good job of sticking to code conventions (see other
> posting). There are a few things that needed tending to in the first patch.
>
> (1) Version changes
> =========> It seems that you applied something like s/\*\*/*/g to the first instance of
> the patch, in order to remove duplicate asterisks. This caused a problem:
>
> --- tfrc_sp_receiver_01.patch   2009/09/12 08:37:12     1.1
> +++ tfrc_sp_receiver_01.patch   2009/09/08 17:34:40
> @@ -1001,8 +1001,8 @@ Index: dccp_tree_work4/net/dccp/ccids/li
>  +
>  +#endif
>  +
> -+extern int  tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry **headp, u64 seqno);
> -+extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry **headp);
> ++extern int  tfrc_sp_tx_hist_add(struct tfrc_tx_hist_entry *headp, u64 seqno);
> ++extern void tfrc_sp_tx_hist_purge(struct tfrc_tx_hist_entry *headp);
>
> I have reverted the bug, also to minimise the difference to the existing (non
> TFRC-SP) files.
>
>
> (2) Other changes that I edited out
> =================> (Other than whitespace changes.)
>
> net/dccp/ccids/lib/loss_interval_sp.c
> -------------------------------------
> I replaced the following dead code
>        if ((tfrc_lh_slab != NULL))
>                return 0;
>
>        if (tfrc_lh_slab != NULL) {
>                kmem_cache_destroy(tfrc_lh_slab);
>                tfrc_lh_slab = NULL;
>        }
>        return -ENOBUFS;
> with
>        return tfrc_lh_slab = NULL ? -ENOBUFS : 0;
>
> Also separated the conditions
> +               if ((len <= 0) ||
> +                   (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))) {
> back into
>                if (len <= 0)
>                        return false;
>
>                if (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))
>                        return false;
>

Thanks!




> I removed the following unnecessary inclusion:
> +#include <linux/random.h>
>

I should not have included that header now, it'll be only necessary in
another patch



>
> The following function pokes a hole in thei so far "abstract" data type;
> the convention has been to access the internals of the struct only via
> get-functions:
>
> static inline struct tfrc_loss_interval
>        *tfrc_lh_get_loss_interval(struct tfrc_loss_hist *lh, const u8 i)
> {
>        BUG_ON(i >= lh->counter);
>        return lh->ring[LIH_INDEX(lh->counter - i - 1)];
> }
>
> (You use it in patch 3/5 to gain access to li_ccval and li_losses.
> Better would be to have two separate accessor functions.)
>

Okay, I will fix this.




>
> net/dccp/ccids/lib/tfrc_equation_sp.c
> -------------------------------------
> This is a prime candidate for removal. After editing out the whitespace
> differences, I found that it is 100% identical with tfrc_equation.c.
>
> The result of this editing has been uploaded to
> http://eden-feed.erg.abdn.ac.uk/tfrc_sp_receiver_01.patch
>
One future patch will need to modify this file, but now it's really an
exact copy.


-- 
Ivo Augusto Andrade Rocha Calado
MSc. Candidate
Embedded Systems and Pervasive Computing Lab - http://embedded.ufcg.edu.br
Systems and Computing Department - http://www.dsc.ufcg.edu.br
Electrical Engineering and Informatics Center - http://www.ceei.ufcg.edu.br
Federal University of Campina Grande - http://www.ufcg.edu.br

PGP: 0x03422935
Quidquid latine dictum sit, altum viditur.

Re: [PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[Ian McDonald]

2009/9/15 Ivo Calado <ivocalado@embedded.ufcg.edu.br>
>
> Hi Gerrit and all.
> Thank you for your fast reply. My comments follow below
>
> >
> > net/dccp/ccids/lib/tfrc_equation_sp.c
> > -------------------------------------
> > This is a prime candidate for removal. After editing out the whitespace
> > differences, I found that it is 100% identical with tfrc_equation.c.
> >
> > The result of this editing has been uploaded to
> > http://eden-feed.erg.abdn.ac.uk/tfrc_sp_receiver_01.patch
> >
> One future patch will need to modify this file, but now it's really an
> exact copy.
>
>
Basically the rule with a patch set is that all the patches should
make sense together.

It may well be that it makes sense to make a copy, but if you want to
do this then present it with the patch that then modifies it.

Ian

Re: [PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[gerrit]

Sorry for the delay in replying.

>> One future patch will need to modify this file, but now it's really an
>> exact copy.
>>
>>
> Basically the rule with a patch set is that all the patches should
> make sense together.
>
> It may well be that it makes sense to make a copy, but if you want to
> do this then present it with the patch that then modifies it.
>
I agree with Ian's point. At the moment I can only see patch 5/5 modifying
this file (adding documentation); from my reading of RFC 4828/5622 it seems
not necessary to use 'tfrc_sp' variants of the functions computing X.

The situation will be better as soon as the patches are in their own subtree.
Currently there is a benefit in using separate files: the tfrc library does
not support a sender-based variant of TFRC, hence requires further work
and/or
a decision to support a sender-bsed variant of CCID-3/4 only in an
experimental subtree - this requires input and discussion.

Re: [PATCH 1/5] First Patch on TFRC-SP. Copy base files from TFRC

[gerrit]

>> Also separated the conditions
>> +               if ((len <= 0) ||
>> +                   (!tfrc_lh_closed_check(cur,
>> cong_evt->tfrchrx_ccval))) {
>> back into
>>                if (len <= 0)
>>                        return false;
>>
>>                if (!tfrc_lh_closed_check(cur, cong_evt->tfrchrx_ccval))
>>                        return false;
>>
>
> Thanks!
Yes I know, the above change is reintroduced by patch 2/2. Only found
out after I had gone through this one.


>> The following function pokes a hole in thei so far "abstract" data type;
>> the convention has been to access the internals of the struct only via
>> get-functions:
>>
>> static inline struct tfrc_loss_interval
>>        *tfrc_lh_get_loss_interval(struct tfrc_loss_hist *lh, const u8 i)
>> {
>>        BUG_ON(i >= lh->counter);
>>        return lh->ring[LIH_INDEX(lh->counter - i - 1)];
>> }
>>
>> (You use it in patch 3/5 to gain access to li_ccval and li_losses.
>> Better would be to have two separate accessor functions.)
>>
>
> Okay, I will fix this.
>
It would be great but is secondary at this stage. The primary objective
should be to get a common prototype out soon, and then verify that it is
correct. I expect several rewrites of other code to make this possible,
so the above detail can also be fixed once a prototype has been found to
work satisfactorily.