[PATCH 5/8] [TFRC]: Loss interval code needs the macros/inlines that were moved

[PATCH 5/8] [TFRC]: Loss interval code needs the macros/inlines that were moved

[Gerrit Renker]

This moves the inlines (which were previously declared as macros) back into packet_history.h since
the loss detection code needs to be able to read entries from the RX history in order to create the
relevant loss entries: it needs at least tfrc_rx_hist_loss_prev() and tfrc_rx_hist_last_rcv(), which
in turn require the definition of the other inlines (macros).

Additionally, inn one case the use of inlines instead of a macro broke the algorithm: rx_hist_swap()
(introduced in next patch) needs to be able to swap the history entries; when using an inline returning
a pointer instead, one gets compilation errors such as:

  distcc[24516] ERROR: compile /root/.ccache/packet_his.tmp.aspire.home.net.24512.i on _tiptop failed
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__one_after_loss':
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:266: error: lvalue required as unary '&' operand
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:267: error: lvalue required as unary '&' operand
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__two_after_loss':
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:298: error: lvalue required as unary '&' operand
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:299: error: lvalue required as unary '&' operand
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:336: error: lvalue required as unary '&' operand
  /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:337: error: lvalue required as unary '&' operand
  make[4]: *** [net/dccp/ccids/lib/packet_history.o] Error 1
  make[3]: *** [net/dccp/ccids/lib] Error 2
  make[2]: *** [net/dccp/ccids] Error 2
  make[1]: *** [net/dccp/] Error 2
  make: *** [sub-make] Error 2

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/lib/packet_history.c |   37 +----------------------------------
 net/dccp/ccids/lib/packet_history.h |   32 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/net/dccp/ccids/lib/packet_history.c b/net/dccp/ccids/lib/packet_history.c
index 6256bec..428636e 100644
--- a/net/dccp/ccids/lib/packet_history.c
+++ b/net/dccp/ccids/lib/packet_history.c
@@ -151,41 +151,6 @@ void __exit tfrc_rx_packet_history_exit(void)
 	}
 }
 
-/**
- * tfrc_rx_hist_index - index to reach n-th entry after loss_start
- */
-static inline u8 tfrc_rx_hist_index(const struct tfrc_rx_hist *h, const u8 n)
-{
-	return (h->loss_start + n) & TFRC_NDUPACK;
-}
-
-/**
- * tfrc_rx_hist_last_rcv - entry with highest-received-seqno so far
- */
-static inline struct tfrc_rx_hist_entry *
-			tfrc_rx_hist_last_rcv(const struct tfrc_rx_hist *h)
-{
-	return h->ring[tfrc_rx_hist_index(h, h->loss_count)];
-}
-
-/**
- * tfrc_rx_hist_entry - return the n-th history entry after loss_start
- */
-static inline struct tfrc_rx_hist_entry *
-		tfrc_rx_hist_entry(const struct tfrc_rx_hist *h, const u8 n)
-{
-	return h->ring[tfrc_rx_hist_index(h, n)];
-}
-
-/**
- * tfrc_rx_hist_loss_prev - entry with highest-received-seqno before loss was detected
- */
-static inline struct tfrc_rx_hist_entry *
-			tfrc_rx_hist_loss_prev(const struct tfrc_rx_hist *h)
-{
-	return h->ring[h->loss_start];
-}
-
 /* has the packet contained in skb been seen before? */
 int tfrc_rx_hist_duplicate(struct tfrc_rx_hist *h, struct sk_buff *skb)
 {
@@ -196,7 +161,7 @@ int tfrc_rx_hist_duplicate(struct tfrc_rx_hist *h, struct sk_buff *skb)
 		return 1;
 
 	for (i = 1; i <= h->loss_count; i++)
-		if (tfrc_rx_hist_entry(h, i)->tfrchrx_seqno = seq)
+		if (TFRC_RX_HIST_ENTRY(h, i)->tfrchrx_seqno = seq)
 			return 1;
 
 	return 0;
diff --git a/net/dccp/ccids/lib/packet_history.h b/net/dccp/ccids/lib/packet_history.h
index 3dfd182..6ea25cd 100644
--- a/net/dccp/ccids/lib/packet_history.h
+++ b/net/dccp/ccids/lib/packet_history.h
@@ -84,6 +84,38 @@ struct tfrc_rx_hist {
 #define rtt_sample_prev		  loss_start
 };
 
+/**
+ * tfrc_rx_hist_index - index to reach n-th entry after loss_start
+ */
+static inline u8 tfrc_rx_hist_index(const struct tfrc_rx_hist *h, const u8 n)
+{
+	return (h->loss_start + n) & TFRC_NDUPACK;
+}
+
+/**
+ * tfrc_rx_hist_last_rcv - entry with highest-received-seqno so far
+ */
+static inline struct tfrc_rx_hist_entry *
+			tfrc_rx_hist_last_rcv(const struct tfrc_rx_hist *h)
+{
+	return h->ring[tfrc_rx_hist_index(h, h->loss_count)];
+}
+
+/**
+ * tfrc_rx_hist_entry - return the n-th history entry after loss_start
+ * This has to be a macro since rx_hist_swap needs to be able to swap entries.
+ */
+#define TFRC_RX_HIST_ENTRY(h, n)	((h)->ring[tfrc_rx_hist_index(h, n)])
+
+/**
+ * tfrc_rx_hist_loss_prev - entry with highest-received-seqno before loss was detected
+ */
+static inline struct tfrc_rx_hist_entry *
+			tfrc_rx_hist_loss_prev(const struct tfrc_rx_hist *h)
+{
+	return h->ring[h->loss_start];
+}
+
 extern void tfrc_rx_hist_add_packet(struct tfrc_rx_hist *h,
 				    const struct sk_buff *skb, const u32 ndp);
 

Re: [PATCH 5/8] [TFRC]: Loss interval code needs the

[Arnaldo Carvalho de Melo]

Em Sat, Dec 08, 2007 at 10:06:25AM +0000, Gerrit Renker escreveu:
> This moves the inlines (which were previously declared as macros) back into packet_history.h since
> the loss detection code needs to be able to read entries from the RX history in order to create the
> relevant loss entries: it needs at least tfrc_rx_hist_loss_prev() and tfrc_rx_hist_last_rcv(), which
> in turn require the definition of the other inlines (macros).
> 
> Additionally, inn one case the use of inlines instead of a macro broke the algorithm: rx_hist_swap()
> (introduced in next patch) needs to be able to swap the history entries; when using an inline returning
> a pointer instead, one gets compilation errors such as:
> 
>   distcc[24516] ERROR: compile /root/.ccache/packet_his.tmp.aspire.home.net.24512.i on _tiptop failed
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__one_after_loss':
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:266: error: lvalue required as unary '&' operand
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:267: error: lvalue required as unary '&' operand
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__two_after_loss':
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:298: error: lvalue required as unary '&' operand
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:299: error: lvalue required as unary '&' operand
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:336: error: lvalue required as unary '&' operand
>   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:337: error: lvalue required as unary '&' operand
>   make[4]: *** [net/dccp/ccids/lib/packet_history.o] Error 1
>   make[3]: *** [net/dccp/ccids/lib] Error 2
>   make[2]: *** [net/dccp/ccids] Error 2
>   make[1]: *** [net/dccp/] Error 2
>   make: *** [sub-make] Error 2

Because you do it this way:

tfrc_rx_hist_swap(&TFRC_RX_HIST_ENTRY(h, 0), &TFRC_RX_HIST_ENTRY(h, 3));

I checked and at least in this patch series all uses are of this type,
so why not do it using just the indexes, which would be simpler:

tfrc_rx_hist_swap(h, 0, 3);

With this implementation:

static void tfrc_rx_hist_swap(struct tfrc_rx_hist *h, const int a, const int b)
{
	const int idx_a = tfrc_rx_hist_index(h, a),
	      int idx_b = tfrc_rx_hist_index(h, b);
	struct tfrc_rx_hist_entry *tmp = h->ring[idx_a];

	h->ring[idx_a] = h->ring[idx_b];
	h->ring[idx_b] = tmp;
}

- Arnaldo

Re: [PATCH 5/8] [TFRC]: Loss interval code needs the macros/inlines that were moved

[Gerrit Renker]

| > 
| >   distcc[24516] ERROR: compile /root/.ccache/packet_his.tmp.aspire.home.net.24512.i on _tiptop failed
| >   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__one_after_loss':
| >   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:266: error: lvalue required as unary '&' operand
<snip>
| 
| Because you do it this way:
| 
| tfrc_rx_hist_swap(&TFRC_RX_HIST_ENTRY(h, 0), &TFRC_RX_HIST_ENTRY(h, 3));
| 
| I checked and at least in this patch series all uses are of this type,
| so why not do it using just the indexes, which would be simpler:
| 
| tfrc_rx_hist_swap(h, 0, 3);
| 
| With this implementation:
| 
| static void tfrc_rx_hist_swap(struct tfrc_rx_hist *h, const int a, const int b)
| {
| 	const int idx_a = tfrc_rx_hist_index(h, a),
| 	      int idx_b = tfrc_rx_hist_index(h, b);
| 	struct tfrc_rx_hist_entry *tmp = h->ring[idx_a];
| 
| 	h->ring[idx_a] = h->ring[idx_b];
| 	h->ring[idx_b] = tmp;
| }
| 
Agreed, that is useful in the present case, since then everything uses
inlines. The only suggestion I'd like to make is to use `u8' instead of 
`int' since the indices will have very low values.

There is a related point: you will probably have noticed that loss_interval.c 
also uses macros. I don't know if you are planning to convert these also into 
inlines. I think that there would be less benefit in converting these, since
they are locl to loss_interval.c and mostly serve to improve readability.

As I have at least one other patch to revise (plus another minor one),
I'll rework this according to the above. 

Re: [PATCH 5/8] [TFRC]: Loss interval code needs the

[Arnaldo Carvalho de Melo]

Em Mon, Dec 10, 2007 at 11:31:53AM +0000, Gerrit Renker escreveu:
> | > 
> | >   distcc[24516] ERROR: compile /root/.ccache/packet_his.tmp.aspire.home.net.24512.i on _tiptop failed
> | >   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c: In function '__one_after_loss':
> | >   /usr/src/davem-2.6/net/dccp/ccids/lib/packet_history.c:266: error: lvalue required as unary '&' operand
> <snip>
> | 
> | Because you do it this way:
> | 
> | tfrc_rx_hist_swap(&TFRC_RX_HIST_ENTRY(h, 0), &TFRC_RX_HIST_ENTRY(h, 3));
> | 
> | I checked and at least in this patch series all uses are of this type,
> | so why not do it using just the indexes, which would be simpler:
> | 
> | tfrc_rx_hist_swap(h, 0, 3);
> | 
> | With this implementation:
> | 
> | static void tfrc_rx_hist_swap(struct tfrc_rx_hist *h, const int a, const int b)
> | {
> | 	const int idx_a = tfrc_rx_hist_index(h, a),
> | 	      int idx_b = tfrc_rx_hist_index(h, b);
> | 	struct tfrc_rx_hist_entry *tmp = h->ring[idx_a];
> | 
> | 	h->ring[idx_a] = h->ring[idx_b];
> | 	h->ring[idx_b] = tmp;
> | }
> | 
> Agreed, that is useful in the present case, since then everything uses
> inlines. The only suggestion I'd like to make is to use `u8' instead of 
> `int' since the indices will have very low values.

Agreed.
 
> There is a related point: you will probably have noticed that loss_interval.c 
> also uses macros. I don't know if you are planning to convert these also into 
> inlines. I think that there would be less benefit in converting these, since
> they are locl to loss_interval.c and mostly serve to improve readability.

In general I'm against using macros for functions, so please always
consider doing things as inlines.

I'll read some more patches today and provide comments as to if I think
it is ok for now to keep it as macros.

> As I have at least one other patch to revise (plus another minor one),
> I'll rework this according to the above. 

Thank you.

- Arnaldo