Re: [tipc-discussion] [PATCH net-next v3 00/12] tipc: create socket FSM using sk_state only

[Jon Maloy]

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 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;
>>> ma...@donjonn.com; Ying Xue
>>> 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 |
>>> 

Re: [tipc-discussion] [PATCH net-next v3 00/12] tipc: create socket FSM using sk_state only

[Parthasarathy Bhuvaragan]

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.

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.
> 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 
>> ;
>> ma...@donjonn.com; Ying Xue 
>> 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  in
>>   patch #7, #11.
>> - Rebase on latest