Message-ID: <20071205093530.GA5177@gerrit.erg.abdn.ac.uk>
Date:	Wed, 5 Dec 2007 09:35:30 +0000
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Arnaldo Carvalho de Melo <acme@...hat.com>, netdev@...r.kernel.org,
	dccp@...r.kernel.org
Subject: Re: [PATCH 7/7] [TFRC] New rx history code

Quoting Arnaldo:
| Em Tue, Dec 04, 2007 at 06:55:17AM +0000, Gerrit Renker escreveu:
| > NAK. You have changed the control flow of the algorithm and the underlying
| > data structure. Originally it had been an array of pointers, and this had
| > been a design decision right from the first submissions in March. From I
| > of http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/loss_detection/loss_detection_algorithm_notes.txt
| >  
| >  "1. 'ring' is an array of pointers rather than a contiguous area in
| >      memory. The reason is that, when having to swap adjacent entries
| >      to sort the history, it is easier to swap pointers than to copy
| >      (heavy-weight) history-entry data structs."
| > 
| > But in your algorithm it becomes a normal linear array.
| > 
| > As a consequence all subsequent code needs to be rewritten to use
| > copying instead of swapping pointers. And there is a lot of that, since
| > the loss detection code makes heavy use of swapping entries in order to
| > cope with reordered and/or delayed packets.
| 
| So lets not do that, the decision was made based on the RX history patch
| alone, where, as pointed out, no swapping occurs.
|  
| > This is not what I would call "entirely equivalent" as you claim. Your
| > algorithm is fundamentally different, the control flow is not the same,
| > nor are the underlying data structures.
| 
| Its is equivalent up to what is in the tree, thank you for point out
| that in subsequent patches it will be used.
| 
| Had this design decision been made explicit in the changeset comment
| that introduces the data structure I wouldn't have tried to reduce the
| number of allocations.
| 
| And you even said that it was a good decision when first reacting to
| this change, which I saw as positive feedback for the RFC I sent.
| 
That was my fault. Your solution "looked" much better to me but even I had forgotten
that the algorithm is based on an array of pointers. When I finally sat down 
and looked through the patch set I found the original notes from March and
saw that using a linear array instead of an array of pointers will require quite
a few changes and means changing the algorithm. I then thought through whether there
could be any advantage in using a linear array as in this patch, but could find none.
In the end this went back to the same questions and issues that were tackled between
February and March this year. I wish we could have had this dicussion then; it would
have made this email unnecessary.

I thank you for your replay and I am sorry if you took offense at my perhaps a little strong
reaction, here is the essence what I tried to convey but what maybe did not succeed in conveying:

 * I am absolutely ok with you making changes to variable names, namespaces, file organisation,
   overall arrangement etc (as per previous email). You have experience and this will often be a 
   benefit. For instance, you combined tfrc_entry_from_skb() with tfrc_rx_hist_update() to give
   a new function, tfrc_rx_hist_add_packet(). This is ok since entry_from_skb() is not otherwise
   used (and I only found this out when reading your patch). (On the other hand, since this is a
   4-element ring buffer, it is no real adding, the same entry will frequently be overwritten,
   this is why it was initially called hist_update().)

 * I am also not so much concerned about credit. As long as the (changed) end result is as
   least as good as or hopefully better than the submitted input, the author name is a side
   issue; so I do think that your approach is sound and fair. The only place where credits
   are needed is for the SatSix project (www.ist-satsix.org) which funded this work. This is
   why some of the copyrights now included "University of Aberdeen".  What in any case I'd like
   to add is at least the Signed-off-by: if it turns out to be a mess I want to contribute my
   part of responsibility.

 * But where I need to interact is when changes are made to the algorithm, even if these may 
   seem small. The underlying reasons that lead to the code may not be immediately clear,
   but since this is a solution for a specific problem, there are also specific reasons; which
   is why for algorithm changes (and underlying data structure) I'd ask you to discuss this first
   before committing the patch. 

 * In part you are already doing this (message subject); it may be necessary to adapt the way
   of discussion/presentation. The subject is complex (spent a week with the flow chart only)
   and it is a lot - initially this had been 40 small patches.	
   
| > | I modified it just to try to make it closer to the existing API, hide details from
| > | the CCIDs and fix a couple bugs found in the initial implementation.
| > What is "a couple bugs"? So far you have pointed out only one problem and that was
| > agreed, it can be fixed. But it remains a side issue, it is not a failure of the
| 
| I pointed two problems one ended up being just the fact that tfrc_ewma
| checks if the average is zero for every calculation.
| 
With regard to the tfrc_ewma we need to consider the discussion as I think we are having a
misunderstanding. It is necessary to 
 (a) deal with the case that a peer may be sending zero-sized packets:
     - one solution is to simply omit the check "if (len > 0)" and declare 0-sized data
     - I think you have a point in that the len check is indeed redundant when we are
       already checking that this is a data packet
       packets as pathological (which is probably what they are);
     - a BUG_ON would be not such a good solution since 0-sized packets are legal;
     - my suggestion is to remove the "len > 0" test - then tfrc_ewma() can also be
       used directly, without the surrounding inline function wrapper.

 (b) reconsider the control flow: in your patch set you introduced a different control flow
     in the rx_packet_recv() function. I tried to figure out why this was introduced, there
     are two problems, one not immediately obvious at this moment:
     - it does not consider where and how loss detection is done (which is part of a later
       patch). This is a key point why the flow may seem strange: loss detection must be
       done on both non-data and data packets; there are several cases depending on whether
       loss previously occurred; the accounting must also be done in parallel for data packets
       (for instance s and bytes_recvd) and non-data packets (highest-received seqno is always
     - it is not equivalent with the initial one, when comparing this to the flowchart:
       http://www.erg.abdn.ac.uk/users/gerrit/dccp/notes/ccid3_packet_reception/reception-main-flowchart.png

| > I have provided documentation, written test modules, and am able to prove
| > that the algorithm as submitted works reasonably correct. In addition, the
| > behaviour has been verified using regression tests.
| > 
| > I am not prepared to take your claims and expressions of "deepest
| > respect" at face value since your actions - not your words - show that
| > you are, in fact, not dealing respectfully with work which has taken
| > months to complete and verify.
| > 
| > During 9 months on dccp@...r you did not provide so much as a paragraph
| > of feedback. Instead you mopped up code from the test tree, modified it
| > according to your own whims and now, in the circle of your
| > invitation-only buddies, start to talk about having discussions and 
| > iterations. The only iteration I can see is in fixing up the things you
| > introduced, so it is not you who has to pay the price.
| > 
| > Sorry, unless you can offer a more mature model of collaboration,
| > consider me out of this and the patches summarily withdrawn. I am not
| > prepared to throw away several months of work just because you feel
| > inspired to do as you please with the work of others. 
| 
| Again you want to have your patches accepted as-is. Pointed to one case
| where I gave you credit while improving on your work (TX history) and
| another where the changes were up for review. I don't consider this a
| warm welcome for me to finally dedicate time to this effort. Would you
| prefer to continue working without help? I think we should encourage
| more people to work on DCCP, you seem to think that changes to your work
| are not acceptable.
| 
Yes and no. The internal algorithm I would rather have accepted as-is, unless
there are good reasons to change it. But this by no means means I want this 
waved through like a diplomat's car at the border: there are reasons why
it is as it is, and I am happy to discuss these reasons; and if someone
can point out a better solution, will accept that. It is just that this
is a solution to a specific problem which so far has proven to work, and
I therefore think that we can spent the time more efficiently by not
fixing things that have already been fixed once: there are many more
problems in DCCP (just recently you mentioned robustness of memory usage,
then there is the ongoing discussion of how to integrate CCID4 as well);
I think we could divide up work time with more benefit.


| Perhaps others can comment on what is happening and help us find, as you
| say, to find a more mature model of collaboration.
| 
No need to be cynical, you have replied gracefully and with substance.
For that I thank you. The topic is complicated, adapting how it is
treated may improve the discussion - for instance, if it helps, we could
switch back to smaller patch format as original, which you have already
been introducing.


Best regards
Gerrit
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists