Hi Partha,
It looks good. I am especially happy that you got rid of the "connected" field, 
which I have been requesting for several years.  
I only have a couple of comments:

1: It disturbs me that TIPC_UNCONNECTED and TIPC_LISTENING sockets goes to 
state TIPC_DISCONNECTING before they are deleted, without ever having been 
connected. This is counterintuitive and cannot be necessary.

2: "connection less" should be "connectionless".  In several places.

3: We really must rename/retype the confusing "handle" field used in 
TIPC_SKB_CB()->handle. I believe it should be made a u32 and called "read" or 
"offset" or similar.  Maybe a separate patch before or after this series?

Regards
///jon


> -----Original Message-----
> From: Parthasarathy Bhuvaragan
> Sent: Tuesday, 13 September, 2016 06:52
> To: tipc-discussion@lists.sourceforge.net; Jon Maloy <jon.ma...@ericsson.com>;
> ma...@donjonn.com; Ying Xue <ying....@windriver.com>
> Subject: [PATCH net-next v3 00/12] tipc: create socket FSM using sk_state only
> 
> The following issues with the current socket layer hinders socket diagnostics
> implementation, which led to this patch series. The series does not add any
> functional change.
> 
> 1. tipc socket state is derived from multiple variables like
>    sock->state, tsk->probing_state and tsk->connected. This style forces
>    us to export multiple attributes to the user space, which has to be
>    backward compatible.
> 
> 2. Abuse of sock->state cannot be exported to user-space without
>    requiring tipc specific hacks in the user-space.
>    - For connection less (CL) sockets sock->state is overloaded to
>      tipc state SS_READY.
>    - For connection oriented (CO) listening socket sock->state is
>      overloaded to tipc state SS_LISTEN.
> 
> This series is split into three:
> 1. A bug fix in patch-1
> 2. Express all tipc states using a single variable. This is done in patch#2-5.
> 3. Migrate the new tipc states to sk->sk_state. This is done in patch#6-12.
> 
> The figures below represents the FSM after this series:
> 
> Unconnected Sockets:
> +------------------+        +--------------------+
> | TIPC_UNCONNECTED |------->| TIPC_DISCONNECTING |
> +------------------+        +--------------------+
> 
> Stream Server Listening Socket:
> +------------------+        +-------------+
> | TIPC_UNCONNECTED |------->| TIPC_LISTEN |
> +------------------+        +-------------+
>                                   |
> +--------------------+            |
> | TIPC_DISCONNECTING |<-----------+
> +--------------------+
> 
> Stream Server Data Socket:
> +-----------------+        +------------------+
> |TIPC_UNCONNECTED |------> | TIPC_ESTABLISHED |<---+
> +-----------------+        +------------------+    |
>                                ^   |    |          |
>                                |   |    +----------+
>                                |   v
> +------------------+      +-------------+
> |TIPC_DISCONNECTING|<-----|TIPC_PROBING |
> +------------------+      +-------------+
> 
> Stream Socket Client:
> +-----------------+       +-----------------+
> |TIPC_UNCONNECTED |------>| TIPC_CONNECTING |
> +-----------------+       +-----------------+
>                                   |
>                                   |
>                                   v
>                           +------------------+
>                           | TIPC_ESTABLISHED |<---+
>                           +------------------+    |
>                                ^   |    |         |
>                                |   |    +---------+
>                                |   v
> +------------------+      +-------------+
> |TIPC_DISCONNECTING|<-----|TIPC_PROBING |
> +------------------+      +-------------+
> 
> NOTE:
> This is just a base refractoring required for socket diagnostics.
> Implementation of TIPC socket diagnostics will be sent as a
> separate series.
> 
> ---
> I plan to submit this series after v4.8 release cycle.
> 
> v3: - Address comments from Ying Xue <ying....@windriver.com> in
>       patch #7, #11.
>     - Rebase on latest netnext which contains fixes for broadcast NACK
>       that seems to make ptts regression stable.
>     - Ran ptts suits for 6000 iterations for 5+ hours.
> 
> v2: - Address comments from Ying Xue <ying....@windriver.com> in
>       patch #4, #5, #12.
>     - Added a note that the socket diagnostics will be sent as
>       another series.
> v1: - I base the following patch series as the first version:
>       [RFC PATCH v1 00/12] tipc: create socket FSM using sk_state only
> 
> Parthasarathy Bhuvaragan (12):
>   tipc: set kern=0 in sk_alloc() during tipc_accept()
>   tipc: rename tsk->remote to tsk->peer for consistent naming
>   tipc: remove tsk->connected for connection less sockets
>   tipc: remove tsk->connected from tipc_sock
>   tipc: remove probing_intv from tipc_sock
>   tipc: remove socket state SS_READY
>   tipc: create TIPC_LISTEN as a new sk_state
>   tipc: create TIPC_PROBING/TIPC_ESTABLISHED as new sk_states
>   tipc: create TIPC_UNCONNECTED as a new sk_state
>   tipc: create TIPC_DISCONNECTING as a new sk_state
>   tipc: create TIPC_CONNECTING as a new sk_state
>   tipc: remove SS_CONNECTED sock state
> 
>  include/uapi/linux/tipc.h |  12 ++
>  net/tipc/socket.c         | 339 
> ++++++++++++++++++++++++++--------------------
>  2 files changed, 203 insertions(+), 148 deletions(-)
> 
> --
> 2.1.4


------------------------------------------------------------------------------
_______________________________________________
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Reply via email to