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

Reply via email to