[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_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.

[PATCHv2 1/4] 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/patches_tfrc_sp_receiver-v2/tfrc_sp_receiver_01.patch

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

[Jarek Poplawski]

Ivo Calado wrote, On 10/13/2009 07:18 PM:

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

Does this patch really exceed 300kb in size?

Jarek P.

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

[Ivo Calado]

On Tue, Oct 13, 2009 at 15:06, Jarek Poplawski <jarkao2@gmail.com> wrote:
> Ivo Calado wrote, On 10/13/2009 07:18 PM:
>
> ...
>> Following the rule #8 in Documentation/SubmittingPatches the patch is
>> stored at
>> http://embedded.ufcg.edu.br/~ivocalado/dccp/patches/patches_tfrc_sp_receiver-v2/tfrc_sp_receiver_01.patch
>>
>
> Does this patch really exceed 300kb in size?
>
> Jarek P.
>


Hi jarek,
  This patch has 68kb in size, but in the rule #8 it says:


8) E-mail size.

...
Large changes are not appropriate for mailing lists, and some
maintainers.  If your patch, uncompressed, exceeds 40 kB in size,
it is preferred that you store your patch on an Internet-accessible
server, and provide instead a URL (link) pointing to your patch.
"


-- 
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: [PATCHv2 1/4] First Patch on TFRC-SP. Copy base files from TFRC

[Jarek Poplawski]

On Tue, Oct 13, 2009 at 03:12:14PM -0300, Ivo Calado wrote:
> On Tue, Oct 13, 2009 at 15:06, Jarek Poplawski <jarkao2@gmail.com> wrote:
> > Ivo Calado wrote, On 10/13/2009 07:18 PM:
> >
> > ...
> >> Following the rule #8 in Documentation/SubmittingPatches the patch is
> >> stored at
> >> http://embedded.ufcg.edu.br/~ivocalado/dccp/patches/patches_tfrc_sp_receiver-v2/tfrc_sp_receiver_01.patch
> >>
> >
> > Does this patch really exceed 300kb in size?
> >
> > Jarek P.
> >
> 
> 
> Hi jarek,
>   This patch has 68kb in size, but in the rule #8 it says:
> 
> 
> 8) E-mail size.
> 
> ...
> Large changes are not appropriate for mailing lists, and some
> maintainers.  If your patch, uncompressed, exceeds 40 kB in size,
> it is preferred that you store your patch on an Internet-accessible
> server, and provide instead a URL (link) pointing to your patch.
> "

Hi Ivo,
Current version?

"8) E-mail size.

When sending patches to Linus, always follow step #7.

Large changes are not appropriate for mailing lists, and some
maintainers.  If your patch, uncompressed, exceeds 300 kB in size,
it is preferred that you store your patch on an Internet-accessible
server, and provide instead a URL (link) pointing to your patch.
"

Jarek P.

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

[Ivo Calado]

On Tue, Oct 13, 2009 at 15:21, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Tue, Oct 13, 2009 at 03:12:14PM -0300, Ivo Calado wrote:
>> On Tue, Oct 13, 2009 at 15:06, Jarek Poplawski <jarkao2@gmail.com> wrote:
>> > Ivo Calado wrote, On 10/13/2009 07:18 PM:
>> >
>> > ...
>> >> Following the rule #8 in Documentation/SubmittingPatches the patch is
>> >> stored at
>> >> http://embedded.ufcg.edu.br/~ivocalado/dccp/patches/patches_tfrc_sp_receiver-v2/tfrc_sp_receiver_01.patch
>> >>
>> >
>> > Does this patch really exceed 300kb in size?
>> >
>> > Jarek P.
>> >
>>
>>
>> Hi jarek,
>>   This patch has 68kb in size, but in the rule #8 it says:
>>
>>
>> 8) E-mail size.
>>
>> ...
>> Large changes are not appropriate for mailing lists, and some
>> maintainers.  If your patch, uncompressed, exceeds 40 kB in size,
>> it is preferred that you store your patch on an Internet-accessible
>> server, and provide instead a URL (link) pointing to your patch.
>> "
>
> Hi Ivo,
> Current version?
>
> "8) E-mail size.
>
> When sending patches to Linus, always follow step #7.
>
> Large changes are not appropriate for mailing lists, and some
> maintainers.  If your patch, uncompressed, exceeds 300 kB in size,
> it is preferred that you store your patch on an Internet-accessible
> server, and provide instead a URL (link) pointing to your patch.
> "
>
> Jarek P.
>


Hi Jarek,
     strange! I'm using the current DCCP test tree, branch dccp.
Am I correct in use these rules?

best regards,

Ivo



-- 
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: [PATCHv2 1/4] First Patch on TFRC-SP. Copy base files from TFRC

[Arnaldo Carvalho de Melo]

Em Tue, Oct 13, 2009 at 03:25:48PM -0300, Ivo Calado escreveu:
>      strange! I'm using the current DCCP test tree, branch dccp.
> Am I correct in use these rules?

Follow the rules in the latest Documentation/SubmittingPatches file,
that is:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob_plain;f=Documentation/SubmittingPatches;hb=HEAD

And that states:

-------------------------------------------------------------------

8) E-mail size.

When sending patches to Linus, always follow step #7.

Large changes are not appropriate for mailing lists, and some
maintainers.  If your patch, uncompressed, exceeds 300 kB in size,
it is preferred that you store your patch on an Internet-accessible
server, and provide instead a URL (link) pointing to your patch.

-------------------------------------------------------------------

As Jarek pointed out.

- Arnaldo

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

[Jarek Poplawski]

On Tue, Oct 13, 2009 at 03:25:48PM -0300, Ivo Calado wrote:
> On Tue, Oct 13, 2009 at 15:21, Jarek Poplawski <jarkao2@gmail.com> wrote:
> > On Tue, Oct 13, 2009 at 03:12:14PM -0300, Ivo Calado wrote:
> >> On Tue, Oct 13, 2009 at 15:06, Jarek Poplawski <jarkao2@gmail.com> wrote:
> >> > Ivo Calado wrote, On 10/13/2009 07:18 PM:
> >> >
> >> > ...
> >> >> Following the rule #8 in Documentation/SubmittingPatches the patch is
...
> >> 8) E-mail size.
> >>
> >> ...
> >> Large changes are not appropriate for mailing lists, and some
> >> maintainers.  If your patch, uncompressed, exceeds 40 kB in size,
...
> > Large changes are not appropriate for mailing lists, and some
> > maintainers.  If your patch, uncompressed, exceeds 300 kB in size,
...
> Hi Jarek,
>      strange! I'm using the current DCCP test tree, branch dccp.
> Am I correct in use these rules?

Hmm... Looks like a fresh change in Linus' (and net-next) tree. So,
there is a hard legal question: which tree rules should be respected
here?

I guess you're one of a few respecting these rules ;-) Well done!

Best regards,
Jarek P.

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

[Gerrit Renker]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

| First Patch on TFRC-SP.
Please find attached one edit that I made.

I added unwinding the initialisation of tfrc_lib in the case where the
initialisation of tfrc_sp_lib fails.  Unwinding is now done in the reverse
order of the steps done during initialisation.

[-- Attachment #2: 1.diff --]
[-- Type: text/x-diff, Size: 689 bytes --]

--- a/net/dccp/ccid.c
+++ b/net/dccp/ccid.c
@@ -223,7 +223,7 @@ int __init ccid_initialize_builtins(void
 
 	err = tfrc_sp_lib_init();
 	if (err)
-		return err;
+		goto unwind_tfrc_lib;
 
 	for (i = 0; i < ARRAY_SIZE(ccids); i++) {
 		err = ccid_activate(ccids[i]);
@@ -235,8 +235,10 @@ int __init ccid_initialize_builtins(void
 unwind_registrations:
 	while(--i >= 0)
 		ccid_deactivate(ccids[i]);
-	tfrc_lib_exit();
+
 	tfrc_sp_lib_exit();
+unwind_tfrc_lib:	
+	tfrc_lib_exit();
 	return err;
 }
 
@@ -246,6 +248,6 @@ void ccid_cleanup_builtins(void)
 
 	for (i = 0; i < ARRAY_SIZE(ccids); i++)
 		ccid_deactivate(ccids[i]);
-	tfrc_lib_exit();
 	tfrc_sp_lib_exit();
+	tfrc_lib_exit();
 }

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

[Ivo Calado]

On Mon, Oct 19, 2009 at 02:21, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> | First Patch on TFRC-SP.
> Please find attached one edit that I made.
>
> I added unwinding the initialisation of tfrc_lib in the case where the
> initialisation of tfrc_sp_lib fails.  Unwinding is now done in the reverse
> order of the steps done during initialisation.
>

Agree.



-- 
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.