Messages in this thread | | | From | xu <> | Subject | RE: [RFC PATCH] net/tipc: reduce tipc_node lock holding time in tipc_rcv | Date | Thu, 23 Nov 2023 12:34:01 +0000 |
| |
>>>This patch is wrong. le->link and link status must be protected by node lock. See what happens if tipc_node_timeout() is called, and >>the link goes down: >>>tipc_node_timeout() >>> tipc_node_link_down() >>> { >>> struct tipc_link *l = le->link; >>> ... >>> if (delete) { >>> kfree(l); >>> le->link = NULL; >>> } >>> ... >>> } >> >>Happy to see your reply. But Why? 'delete' is false from tipc_node_timeout(). Refer to: >>https://elixir.bootlin.com/linux/v6.7-rc2/source/net/tipc/node.c#L844 >I should have explained it clearly:
Thanks. I see, so the root reason for holding node lock before tipc_link_rcv is to protect links from being reset or deleted when tipc_link_rcv().
For further discussion, to balance incoming packets (the same links, different ports) to multi-CPUs, maybe we can try RCU + spinlock here.
>1/ link status must be protected. >tipc_node_timeout() > tipc_node_link_down() > { > struct tipc_link *l = le->link; > > ... > __tipc_node_link_down(); <-- link status is referred. > ... > if (delete) { > kfree(l); > le->link = NULL; > } > ... > } > >__tipc_node_link_down() >{ > ... > if (!l || tipc_link_is_reset(l)) <-- read link status > ... > tipc_link_reset(l); <--- this function will reset all things related to link. >} > >2/ le->link must be protected. >bearer_disable() >{ > ... > tipc_node_delete_links(net, bearer_id); <--- this will delete all links. > ... >} > >tipc_node_delete_links() >{ > ... > tipc_node_link_down(n, bearer_id, true); > ... >}
| |