[RFC-RESEND] [PATCH]: use explicit enums for CCID 3 states

[RFC-RESEND] [PATCH]: use explicit enums for CCID 3 states

[Gerrit Renker]

Hi Arnaldo,

thank you for the link @-)

|  > The following is the revised patch with three macros added, I liked the
|  > DCCP_BUG_ON():
|  
|  will look into that barring social life that is taking me so much time
|  that I should be working on DCCP :-P :-) Well, at least its not "Real
|  Work (tm)" :-)
I really hope you can get some quality time - there should be funding for all the
good work you are putting into this. 

I have made a terrible mess of the previous patch (with the warn semantics,
it would not have worked correctly). I have tested it and below is what I
had intended, but was too tired to get clear.

Best,
Gerrit

--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -18,6 +18,14 @@
 #include <net/tcp.h>
 #include "ackvec.h"
 
+#define DCCP_CRIT(fmt, a...) LIMIT_NETDEBUG(KERN_CRIT fmt " at %s:%d/%s()\n",  \
+					 ##a, __FILE__, __LINE__, __FUNCTION__)
+#define DCCP_BUG(fmt, a...)  do { DCCP_CRIT(fmt, ##a); dump_stack(); } while (0)
+#define DCCP_BUG_ON(cond)    do { if (unlikely((cond) = 0))                   \
+					DCCP_BUG("BUG: condition \"%s\" fails",\
+						 __stringify((cond)));	       \
+			     } while (0)
+
 #ifdef MODULE
 #define DCCP_PRINTK(enable, fmt, args...)	do { if (enable)	     \
 							printk(fmt, ##args); \
--- a/net/dccp/ccids/ccid3.h
+++ b/net/dccp/ccids/ccid3.h
@@ -73,6 +73,14 @@ struct ccid3_options_received {
 	u32 ccid3or_receive_rate;
 };
 
+/* TFRC sender states */
+enum ccid3_hc_tx_states {
+       	TFRC_SSTATE_NO_SENT = 1,
+	TFRC_SSTATE_NO_FBACK,
+	TFRC_SSTATE_FBACK,
+	TFRC_SSTATE_TERM,
+};
+
 /** struct ccid3_hc_tx_sock - CCID3 sender half connection sock
  *
   * @ccid3hctx_state - Sender state
@@ -103,7 +111,7 @@ struct ccid3_hc_tx_sock {
 #define ccid3hctx_t_rto			ccid3hctx_tfrc.tfrctx_rto
 #define ccid3hctx_t_ipi			ccid3hctx_tfrc.tfrctx_ipi
 	u16				ccid3hctx_s;
-  	u8				ccid3hctx_state;
+  	enum ccid3_hc_tx_states 	ccid3hctx_state:8;
 	u8				ccid3hctx_last_win_count;
 	u8				ccid3hctx_idle;
 	struct timeval			ccid3hctx_t_last_win_count;
@@ -115,6 +123,13 @@ struct ccid3_hc_tx_sock {
 	struct ccid3_options_received	ccid3hctx_options_received;
 };
 
+/* TFRC receiver states */
+enum ccid3_hc_rx_states {
+       	TFRC_RSTATE_NO_DATA = 1,
+	TFRC_RSTATE_DATA,
+	TFRC_RSTATE_TERM    = 127,
+};
+
 struct ccid3_hc_rx_sock {
 	struct tfrc_rx_info	ccid3hcrx_tfrc;
 #define ccid3hcrx_x_recv	ccid3hcrx_tfrc.tfrcrx_x_recv
@@ -122,8 +137,8 @@ struct ccid3_hc_rx_sock {
 #define ccid3hcrx_p		ccid3hcrx_tfrc.tfrcrx_p
   	u64			ccid3hcrx_seqno_nonloss:48,
 				ccid3hcrx_ccval_nonloss:4,
-				ccid3hcrx_state:8,
 				ccid3hcrx_ccval_last_counter:4;
+	enum ccid3_hc_rx_states	ccid3hcrx_state:8;
   	u32			ccid3hcrx_bytes_recv;
   	struct timeval		ccid3hcrx_tstamp_last_feedback;
   	struct timeval		ccid3hcrx_tstamp_last_ack;
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -73,14 +73,6 @@ static struct dccp_tx_hist *ccid3_tx_his
 static struct dccp_rx_hist *ccid3_rx_hist;
 static struct dccp_li_hist *ccid3_li_hist;
 
-/* TFRC sender states */
-enum ccid3_hc_tx_states {
-       	TFRC_SSTATE_NO_SENT = 1,
-	TFRC_SSTATE_NO_FBACK,
-	TFRC_SSTATE_FBACK,
-	TFRC_SSTATE_TERM,
-};
-
 #ifdef CONFIG_IP_DCCP_CCID3_DEBUG
 static const char *ccid3_tx_state_name(enum ccid3_hc_tx_states state)
 {
@@ -249,9 +241,8 @@ static void ccid3_hc_tx_no_feedback_time
 						      hctx->ccid3hctx_x));
 		break;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hctx->ccid3hctx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!", dccp_role(sk), sk,
+			 hctx->ccid3hctx_state);
 		goto out;
 	}
 
@@ -327,9 +318,8 @@ static int ccid3_hc_tx_send_packet(struc
 		rc = delay > 0 ? delay : 0;
 		break;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hctx->ccid3hctx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!", dccp_role(sk), sk,
+			 hctx->ccid3hctx_state);
 		rc = -EINVAL;
 		break;
 	}
@@ -421,9 +411,8 @@ static void ccid3_hc_tx_packet_sent(stru
 		}
 		break;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hctx->ccid3hctx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!", dccp_role(sk), sk,
+			 hctx->ccid3hctx_state);
 		break;
 	}
 }
@@ -566,9 +555,8 @@ static void ccid3_hc_tx_packet_recv(stru
 		hctx->ccid3hctx_idle = 1;   
 		break;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hctx->ccid3hctx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!", dccp_role(sk), sk,
+			 hctx->ccid3hctx_state);
 		break;
 	}
 }
@@ -686,13 +674,6 @@ static void ccid3_hc_tx_exit(struct sock
  * RX Half Connection methods
  */
 
-/* TFRC receiver states */
-enum ccid3_hc_rx_states {
-       	TFRC_RSTATE_NO_DATA = 1,
-	TFRC_RSTATE_DATA,
-	TFRC_RSTATE_TERM    = 127,
-};
-
 #ifdef CONFIG_IP_DCCP_CCID3_DEBUG
 static const char *ccid3_rx_state_name(enum ccid3_hc_rx_states state)
 {
@@ -742,9 +723,8 @@ static void ccid3_hc_rx_send_feedback(st
 	}
 		break;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hcrx->ccid3hcrx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!", dccp_role(sk), sk,
+			 hcrx->ccid3hcrx_state);
 		return;
 	}
 
@@ -1086,9 +1066,8 @@ static void ccid3_hc_rx_packet_recv(stru
 		}
 		return;
 	default:
-		printk(KERN_CRIT "%s: %s, sk=%p, Illegal state (%d)!\n",
-		       __FUNCTION__, dccp_role(sk), sk, hcrx->ccid3hcrx_state);
-		dump_stack();
+		DCCP_BUG("%s, sk=%p, Illegal state (%d)!",  dccp_role(sk), sk,
+			 hcrx->ccid3hcrx_state);
 		return;
 	}
 

Re: [RFC-RESEND] [PATCH]: use explicit enums for CCID 3 states

[Arnaldo Carvalho de Melo]

On 11/15/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Hi Arnaldo,
>
> thank you for the link @-)
>
> |  > The following is the revised patch with three macros added, I liked the
> |  > DCCP_BUG_ON():
> |
> |  will look into that barring social life that is taking me so much time
> |  that I should be working on DCCP :-P :-) Well, at least its not "Real
> |  Work (tm)" :-)
> I really hope you can get some quality time - there should be funding for all the
> good work you are putting into this.

Thanks, Mandriva allows me to work part time on what I think is
important, barring when there is some important customer project, that
was the case for at least 6 months this year, when I mostly
disappeared from my kernel projects.

> I have made a terrible mess of the previous patch (with the warn semantics,
> it would not have worked correctly). I have tested it and below is what I
> had intended, but was too tired to get clear.

:-)

Seems ok now:

[acme@newtoy net-2.6.20]$ codiff --verbose /tmp/ccid3.o.before /tmp/ccid3.o.enum
/pub/scm/linux/kernel/git/acme/net-2.6.20/net/dccp/ccids/ccid3.c:
  struct ccid3_hc_tx_sock       |   +0
    ccid3hctx_state;
     from: u8                    /*    30(0)     1(0) */
     to:   enum ccid3_hc_tx_states /*    28(8)     4(8) */
  struct ccid3_hc_rx_sock       |   +0
    ccid3hcrx_state:8;
     from: u64                   /*    16(36)     8(8) */
     to:   enum ccid3_hc_rx_states /*    16(0)     4(8) */
    ccid3hcrx_ccval_last_counter:4;
     from: u64                   /*    16(32)     8(4) */
     to:   u64                   /*    16(40)     8(4) */
 2 structs changed
  ccid3_hc_rx_set_state         |  -28
  ccid3_hc_rx_send_feedback     |  +26
  ccid3_hc_rx_init              |  -11
  ccid3_hc_tx_no_feedback_timer |  +32
  ccid3_hc_tx_packet_recv       |  +32
  ccid3_hc_rx_packet_recv       |  -16
  ccid3_hc_rx_get_info          |   -4
  ccid3_hc_tx_send_packet       |  +27
  ccid3_hc_tx_packet_sent       |  +32
 9 functions changed, 149 bytes added, 59 bytes removed

Thanks, applied.

- Arnaldo

Re: [RFC-RESEND] [PATCH]: use explicit enums for CCID 3 states

[Gerrit Renker]

Hi Arnaldo,

|  Thanks, applied.
many thanks for applying this; it was sooner as expected and I think it would
be good to tidy this up and make it consistent throughout the code. Since I initiated this,
I feel a bit obliged to improve this.

In particular, I have thought about the following scheme which divides warning messages into
two (plus debug) categories

a) GRADE 1: Severe Protocol/Internal Faults
            This includes 
		* violated pre- / postconditions, 
		* illegal states
		* malfunctioning (something is NULL)
   I think these messages should not be rate-limited and, catching up on the discussion,
   should dump the stack after the warning message. 

b) GRADE 2: Plain Misbehaviour
		* out-of-memory messages
		* Request floods
		* protocol violations (wrong message/option format)

To make this consistent, I would like input whether the following is ok with you:

	1) use DCCP_BUG for all Grade 1 type of messages, i.e. it would replace 
		* BUG()
		* WARN_ON() (of course with the appropriate message)
		* all sections using 'printk(KERN_CRIT ... ); dump_stack()'

	2) either keep the LIMIT_NETDEBUG(KERN_WARNING ... ) for Grade 2 or
           put it into a macro which would add the __FUNCTION__ thing to it
 	   
The point is that, if it is done consistently, (1) can later be replaced with the more
conventional BUG() or WARN_ON() once everyone is sure that the code behaves exactly as it shoulds.
For the moment, as Ian has pointed out, bringing the machine to a grinding halt for every little
problem is probably too much of a nuisance (in the same vein, I would really like to suggest the
return of Unload Hack :0).

If you are ok with this basic scheme, I shall set about and tidy things up, otherwise please let
me know what the constraints should be with regard to warning/bug messages.

Best,
Gerrit

Re: [RFC-RESEND] [PATCH]: use explicit enums for CCID 3 states

[Arnaldo Carvalho de Melo]

On 11/16/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Hi Arnaldo,
>
> |  Thanks, applied.
> many thanks for applying this; it was sooner as expected and I think it would
> be good to tidy this up and make it consistent throughout the code. Since I initiated this,
> I feel a bit obliged to improve this.

:)

> In particular, I have thought about the following scheme which divides warning messages into
> two (plus debug) categories
>
> a) GRADE 1: Severe Protocol/Internal Faults
>             This includes
>                 * violated pre- / postconditions,
>                 * illegal states
>                 * malfunctioning (something is NULL)
>    I think these messages should not be rate-limited and, catching up on the discussion,
>    should dump the stack after the warning message.
>
> b) GRADE 2: Plain Misbehaviour
>                 * out-of-memory messages
>                 * Request floods
>                 * protocol violations (wrong message/option format)
>
> To make this consistent, I would like input whether the following is ok with you:
>
>         1) use DCCP_BUG for all Grade 1 type of messages, i.e. it would replace
>                 * BUG()
>                 * WARN_ON() (of course with the appropriate message)
>                 * all sections using 'printk(KERN_CRIT ... ); dump_stack()'
>
>         2) either keep the LIMIT_NETDEBUG(KERN_WARNING ... ) for Grade 2 or
>            put it into a macro which would add the __FUNCTION__ thing to it
>
> The point is that, if it is done consistently, (1) can later be replaced with the more
> conventional BUG() or WARN_ON() once everyone is sure that the code behaves exactly as it shoulds.

Using a DCCP_ prefix for BUG(), BUG_ON(), WARN_ON() and a new WARN()
with the difference being none would panic the machine, but warn, in
some cases ratelimited, seems to be the way to go, later, as you
suggests, we just remove the prefix.

> For the moment, as Ian has pointed out, bringing the machine to a grinding halt for every little
> problem is probably too much of a nuisance (in the same vein, I would really like to suggest the
> return of Unload Hack :0).

I'll look at what to do to avoid allowing the removal while there are
TW sockets on the death row :-)

> If you are ok with this basic scheme, I shall set about and tidy things up, otherwise please let
> me know what the constraints should be with regard to warning/bug messages.
>
> Best,
> Gerrit
>