On Mon, Jan 6, 2025 at 9:12 AM Ales Musil <[email protected]> wrote:
>
>
>
> On Mon, Dec 16, 2024 at 7:27 PM <[email protected]> wrote:
>>
>> From: Numan Siddique <[email protected]>
>>
>> When the ovn-controller main thread has locked the pinctrl mutex,
>> pinctrl thread will not be able to process packet-ins (like DHCP,
>> DNS and few others) since it tries to acquire this mutex for
>> other purposes (besides packet-in processing) - like ip_mcast
>> snoop run and svc monitor runs etc. Its better to try
>> acquiring this mutex so that we can continue processing
>> the packet-ins which don't require this mutex.
>>
>> Signed-off-by: Numan Siddique <[email protected]>
>> ---
>
>
> Hi Numan,
>
> thank you for the patch.
>
> I was wondering if we could solve all this contention by using
> "lockless" structures for all data that require some protection.
> We could replace maps with cmaps, and use RCU if there is any
> other storage instead of hmap. WDYT?
Hi Ales,
Thanks for the review and comments. I think it's a good suggestion.
>
> Since the scope of the change proposed above would be a more
> long term solution I'm probably fine with having this patch as
> intermittent solution.
I'd prefer the same. i.e to replace hmap with cmap in a separate patch set.
I have just one question down below.
>
>> controller/pinctrl.c | 65 ++++++++++++++++++++++++++------------------
>> 1 file changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 3fb7e2fd7..bdf1d151d 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -3885,10 +3885,14 @@ pinctrl_handler(void *arg_)
>>
>> while (!latch_is_set(&pctrl->pinctrl_thread_exit)) {
>> long long int bfd_time = LLONG_MAX;
>> + bool lock_failed = false;
>>
>> - ovs_mutex_lock(&pinctrl_mutex);
>> - ip_mcast_snoop_run();
>> - ovs_mutex_unlock(&pinctrl_mutex);
>> + if (!ovs_mutex_trylock(&pinctrl_mutex)) {
>> + ip_mcast_snoop_run();
>> + ovs_mutex_unlock(&pinctrl_mutex);
>> + } else {
>> + lock_failed = true;
>> + }
>>
>> rconn_run(swconn);
>> new_seq = seq_read(pinctrl_handler_seq);
>> @@ -3913,35 +3917,44 @@ pinctrl_handler(void *arg_)
>> }
>>
>> if (may_inject_pkts()) {
>> - ovs_mutex_lock(&pinctrl_mutex);
>> - send_garp_rarp_run(swconn, &send_garp_rarp_time);
>> - send_ipv6_ras(swconn, &send_ipv6_ra_time);
>> - send_ipv6_prefixd(swconn, &send_prefixd_time);
>> - send_mac_binding_buffered_pkts(swconn);
>> - bfd_monitor_send_msg(swconn, &bfd_time);
>> - ovs_mutex_unlock(&pinctrl_mutex);
>> -
>> + if (!ovs_mutex_trylock(&pinctrl_mutex)) {
>> + send_garp_rarp_run(swconn, &send_garp_rarp_time);
>> + send_ipv6_ras(swconn, &send_ipv6_ra_time);
>> + send_ipv6_prefixd(swconn, &send_prefixd_time);
>> + send_mac_binding_buffered_pkts(swconn);
>> + bfd_monitor_send_msg(swconn, &bfd_time);
>> + ovs_mutex_unlock(&pinctrl_mutex);
>> + } else {
>> + lock_failed = true;
>> + }
>> ip_mcast_querier_run(swconn, &send_mcast_query_time);
>> }
>>
>> - ovs_mutex_lock(&pinctrl_mutex);
>> - svc_monitors_run(swconn, &svc_monitors_next_run_time);
>> - ovs_mutex_unlock(&pinctrl_mutex);
>> + if (!ovs_mutex_trylock(&pinctrl_mutex)) {
>> + svc_monitors_run(swconn, &svc_monitors_next_run_time);
>> + ovs_mutex_unlock(&pinctrl_mutex);
>> + } else {
>> + lock_failed = true;
>> + }
>> }
>>
>> - rconn_run_wait(swconn);
>> - rconn_recv_wait(swconn);
>> - if (rconn_is_connected(swconn)) {
>> - send_garp_rarp_wait(send_garp_rarp_time);
>> - ipv6_ra_wait(send_ipv6_ra_time);
>> - ip_mcast_querier_wait(send_mcast_query_time);
>> - svc_monitors_wait(svc_monitors_next_run_time);
>> - ipv6_prefixd_wait(send_prefixd_time);
>> - bfd_monitor_wait(bfd_time);
>> - }
>> - seq_wait(pinctrl_handler_seq, new_seq);
>> + if (lock_failed) {
>> + poll_immediate_wake();
>
>
> This will effectively degrade the lock into spinlock.
> Are we ok with that? Or how about scheduling the next wake
> in a few ms e.g. 'poll_timer_wait_until(5)'?
I had the same concern. If ovn-controller main thread holds the lock
for a long time, the pinctrl thread will be in spinlock mode.
My main intention was not to delay the pinctrl packets any further than needed.
I think scheduling the next wake after a ms should be o.k.
In the next version, I'll do as you suggested.
Thanks
Numan
>
>>
>> + } else {
>> + rconn_run_wait(swconn);
>> + rconn_recv_wait(swconn);
>> + if (rconn_is_connected(swconn)) {
>> + send_garp_rarp_wait(send_garp_rarp_time);
>> + ipv6_ra_wait(send_ipv6_ra_time);
>> + ip_mcast_querier_wait(send_mcast_query_time);
>> + svc_monitors_wait(svc_monitors_next_run_time);
>> + ipv6_prefixd_wait(send_prefixd_time);
>> + bfd_monitor_wait(bfd_time);
>> + }
>> + seq_wait(pinctrl_handler_seq, new_seq);
>>
>> - latch_wait(&pctrl->pinctrl_thread_exit);
>> + latch_wait(&pctrl->pinctrl_thread_exit);
>> + }
>> poll_block();
>> }
>>
>> --
>> 2.39.3 (Apple Git-146)
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
>
> Thanks,
> Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
