: Macro for moving average
PATCH 24/25 : Macro for moving average
[CCID 3]: Macro for moving average
The moving average computation occurs so frequently in the CCID 3 code that it merits a macro of its own.
--- net/dccp/ccids/ccid3.c | 8 +++----- net/dccp/ccids/lib/tfrc.h | 8 ++++++++ 2 files changed, 11 insertions(+), 5 deletions(-)
net/dccp/ccids/lib/tfrc.h +8 -0
@@ -37,6 +37,14 @@ static inline u32 scaled_div32(u64 a, u3return result; } +/** +* Macro for exponentially weighted moving average +* @weight: Weight to be used as damping factor, in units of 1/10 +* Beware that @val is evaluated multiple times. +*/ +#define TFRC_EWMA (val, newval, weight) \ + val = val? ((weight) * val + (10 - (weight)) * (newval)) / 10 : (newval) +extern u32 tfrc_calc_x (u16 s,u32 R ,u32 p);extern u32 tfrc_calc_x_reverse_lookup (u32 fvalue);
net/dccp/ccids/ccid3.c +3 -5
@@ -183,7 +183,7 @@ static inline void ccid3_hc_tx_update_s( {const u16 old_s = hctx->ccid3hctx_s; - hctx->ccid3hctx_s = old_s =0 ? len : (9 * old_s + len) /10 ; +TFRC_EWMA (hctx->ccid3hctx_s, len,9 );if (hctx->ccid3hctx_s != old_s)ccid3_update_send_interval (hctx);
@@ -474,8 +474,7 @@ static void ccid3_hc_tx_packet_recv(stru* * q is a constant, RFC 3448 recommends 0.9 */ - hctx->ccid3hctx_rtt = hctx->ccid3hctx_rtt =0 ? r_sample - : (9 * hctx->ccid3hctx_rtt + r_sample) /10 ; +TFRC_EWMA (hctx->ccid3hctx_rtt, r_sample,9 );/* * Update allowed sending rate as per draft rfc3448bis, 4.2/4.3
@@ -724,8 +723,7 @@ static inline void ccid3_hc_rx_update_s(if (unlikely (len =0 ))/* don't update on empty packets (e.g. ACKs) */ ccid3_pr_debug ("Packet payload length is 0 - not updating\n" );else - hcrx->ccid3hcrx_s = hcrx->ccid3hcrx_s =0 ? len : - (9 * hcrx->ccid3hcrx_s + len) /10 ; +TFRC_EWMA (hcrx->ccid3hcrx_s, len,9 ); }static void ccid3_hc_rx_send_feedback (struct sock *sk)
Quoted 15 lines · Gerrit Renker
[CCID 3]: Macro for moving average
The moving average computation occurs so frequently in the CCID 3 code that it merits a macro of its own.
Signed-off-by Gerrit Renker <[email protected]>--- net/dccp/ccids/ccid3.c | 8 +++----- net/dccp/ccids/lib/tfrc.h | 8 ++++++++ 2 files changed, 11 insertions(+), 5 deletions(-)net/dccp/ccids/lib/tfrc.h +0
-0@@ -37,6 +37,14 @@ static inline u32 scaled_div32(u64 a, u3return result;
net/dccp/ccids/lib/tfrc.h static inline u32 scaled_div32(u64 a, u3 · line 47
Quoted 10 lines
net/dccp/ccids/lib/tfrc.h +8
-0} +/** +* Macro for exponentially weighted moving average +* @weight: Weight to be used as damping factor, in units of 1/10 +* Beware that @val is evaluated multiple times. +*/ +#define TFRC_EWMA (val, newval, weight) \ + val = val? ((weight) * val + (10 - (weight)) * (newval)) / 10 : (newval) +
Just wondering why you pass weight when you only use 9 anyway and it just adds a step. Is it used in following patches with a different weight?
Ian
Signature · 3 lines
Web: http://wand.net.nz/~iam4 Blog: http://iansblog.jandi.co.nz WAND Network Research Group
Quoting Ian McDonald:
| > +/** | > + * Macro for exponentially weighted moving average | > + * @weight: Weight to be used as damping factor, in units of 1/10 | > + * Beware that @val is evaluated multiple times. | > + */ | > +#define TFRC_EWMA(val, newval, weight) \ | > + val = val? ((weight) * val + (10 - (weight)) * (newval)) / 10 : (newval) | > + | Just wondering why you pass weight when you only use 9 anyway and it | just adds a step. Is it used in following patches with a different | weight? |
In the following patches it is used one more time, which is for the Preventing Oscillations' changeset (this is not in the next patch set but in the one after that); here again the weight 9/10 is used. I think it is useful to keep the weight' argument, since RFC 3448 suggests to use a different weight for the RTT sample when the `Oscillation Prevention' e.g. is not deployed (a q close to zero).
I think it is better to keep the weight argument, it allows changing the code later, while still providing the abstraction: for CCID 3 there are 4 uses of this macro alone.
Quoted 13 lines · Gerrit Renker
Quoting Ian McDonald:
| > +/** | > + * Macro for exponentially weighted moving average | > + * @weight: Weight to be used as damping factor, in units of 1/10 | > + * Beware that @val is evaluated multiple times. | > + */ | > +#define TFRC_EWMA(val, newval, weight) \ | > + val = val? ((weight) * val + (10 - (weight)) * (newval)) / 10 : (newval) | > + | Just wondering why you pass weight when you only use 9 anyway and it | just adds a step. Is it used in following patches with a different | weight? |
Quoted 8 lines
In the following patches it is used one more time, which is for the
Preventing Oscillations' changeset (this is not in the next patch set but in the one after that); here again the weight 9/10 is used. I think it is useful to keep theweight' argument, since RFC 3448 suggests to use a different weight for the RTT sample when the `Oscillation Prevention' e.g. is not deployed (a q close to zero).
I think it is better to keep the weight argument, it allows changing the code later, while still providing the abstraction: for CCID 3 there are 4 uses of this macro alone.
OK.
Signature · 3 lines
Web: http://wand.net.nz/~iam4/ Blog: http://iansblog.jandi.co.nz WAND Network Research Group
Quoted 22 lines · Ian McDonald
Quoted 13 lines · Gerrit Renker
Quoting Ian McDonald:
| > +/** | > + * Macro for exponentially weighted moving average | > + * @weight: Weight to be used as damping factor, in units of 1/10 | > + * Beware that @val is evaluated multiple times. | > + */ | > +#define TFRC_EWMA(val, newval, weight) \ | > + val = val? ((weight) * val + (10 - (weight)) * (newval)) / 10 : (newval) | > + | Just wondering why you pass weight when you only use 9 anyway and it | just adds a step. Is it used in following patches with a different | weight? |Quoted 8 lines
In the following patches it is used one more time, which is for the
Preventing Oscillations' changeset (this is not in the next patch set but in the one after that); here again the weight 9/10 is used. I think it is useful to keep theweight' argument, since RFC 3448 suggests to use a different weight for the RTT sample when the `Oscillation Prevention' e.g. is not deployed (a q close to zero).
I think it is better to keep the weight argument, it allows changing the code later, while still providing the abstraction: for CCID 3 there are 4 uses of this macro alone.
Quoted 3 lines
OK.
Acked-by Ian McDonald <[email protected]>
I don't like macros that receive as a parameter something that will be changed, its more readable to have it as a inline function and return the new value, even if the resulting code uses more characters, so I'm applying the attached patch and will take care of the fallout, that is small from what I grepped at the_whole_lot.
- Arnaldo
net/dccp/ccids/ccid3.c +3 -5
@@ -183,7 +183,7 @@ static inline void ccid3_hc_tx_update_s(struct ccid3_hc_tx_sock *hctx, int len) {const u16 old_s = hctx->ccid3hctx_s; - hctx->ccid3hctx_s = old_s ==0 ? len : (9 * old_s + len) /10 ; + hctx->ccid3hctx_s =tfrc_ewma (hctx->ccid3hctx_s, len,9 );if (hctx->ccid3hctx_s != old_s)ccid3_update_send_interval (hctx);
@@ -474,8 +474,7 @@ static void ccid3_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)* * q is a constant, RFC 3448 recommends 0.9 */ - hctx->ccid3hctx_rtt = hctx->ccid3hctx_rtt ==0 ? r_sample - : (9 * hctx->ccid3hctx_rtt + r_sample) /10 ; + hctx->ccid3hctx_rtt =tfrc_ewma (hctx->ccid3hctx_rtt, r_sample,9 );/* * Update allowed sending rate as per draft rfc3448bis, 4.2/4.3
@@ -724,8 +723,7 @@ static inline void ccid3_hc_rx_update_s(struct ccid3_hc_rx_sock *hcrx, int len)if (unlikely (len ==0 ))/* don't update on empty packets (e.g. ACKs) */ ccid3_pr_debug ("Packet payload length is 0 - not updating\n" );else - hcrx->ccid3hcrx_s = hcrx->ccid3hcrx_s ==0 ? len : - (9 * hcrx->ccid3hcrx_s + len) /10 ; + hcrx->ccid3hcrx_s =tfrc_ewma (hcrx->ccid3hcrx_s, len,9 ); }static void ccid3_hc_rx_send_feedback (struct sock *sk)
net/dccp/ccids/lib/tfrc.h +11 -0
@@ -37,6 +37,17 @@ static inline u32 scaled_div32(u64 a, u32 b)return result; } +/** +* Exponentially weighted moving average +* @weight: Weight to be used as damping factor, in units of 1/10 +*/ +static inline u32 tfrc_ewma (const u32 val,const u32 newval,const u8 weight) +{ +if (val !=0 ) +return (weight * val + (10 - weight) * newval) /10 ; +return newval; +} +extern u32 tfrc_calc_x (u16 s,u32 R ,u32 p);extern u32 tfrc_calc_x_reverse_lookup (u32 fvalue);
| I don't like macros that receive as a parameter something that will be | changed, its more readable to have it as a inline function and return | the new value, even if the resulting code uses more characters, so I'm | applying the attached patch and will take care of the fallout, that is | small from what I grepped at the_whole_lot.
You are right (please see other comment), I wasn't overly happy with this one. Your change makes it much better and avoids future problems due to someone misinterpreting the macro arguments. Many thanks indeed.