On 09/15/2016 02:39 PM, Jon Maloy wrote: > > > On 09/15/2016 08:29 AM, Parthasarathy Bhuvaragan wrote: >> >> On 09/14/2016 10:20 PM, Jon Maloy wrote: >>> 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. >> Changing the names of the new states should make the transitions more logical >> as below: >> TIPC_UNCONNECTED => TIPC_OPEN >> TIPC_DISCONNECTING => TIPC_CLOSE >> So in this case, all open sockets needs to be closed. Its valid for both >> connectionless and connection oriented sockets as for connection oriented >> sockets there is no difference between DISCONNECTING and CLOSE. > > Sounds better, although I think TIPC_CLOSING, or maybe TIPC_RELEASING would be > even better. I realize that its better to keep the state TIPC_DISCONNECTING and create a new state called TIPC_CLOSE. TIPC_DISCONNECTING is set at tipc_shutdown() or when connection probe messages timeout or when we receive an error message on the connection. TIPC_CLOSE is set in all other cases. > >> >> I need that we get names for the states correct, as I will export them to >> user space for socket diagnostics. >> >> BTW, why do we even support shutdown() on connectionless sockets? The >> function handles states relevant only for connection oriented sockets. >>> 2: "connection less" should be "connectionless". In several places. >> I will fix this. I need to fix the description in tipc_poll() too. > > When you have done this and the above name changes, you can add an Acked-by: > from me. > > ///jon > >>> 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? >> I will look into that, and post it separately. >>> 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