| Message ID | 20220818140038.4148633-1-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pinctrl fix missing packets (e.g. icmp) | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Xavier. I think the only foolproof way to get the seq logic right in pinctrl is to perform all of the processing of pinctrl_handler (including reading the seq and waiting on the seq) with the pinctrl_mutex locked. However, this could cause a lot of waiting that we'd like to avoid. I think this change of reading the seq value early is the best compromise. As you mentioned, we could end up getting woken up for nothing, but it's better to be woken up for nothing than to be asleep when we should be processing something. Acked-by: Mark Michelson <mmichels@redhat.com> On 8/18/22 10:00, Xavier Simonart wrote: > seq_read should be used before processing any changes. > If buffered_mac_bindings is updated by pinctrl_run while pinctrl_handler > is running, but before seq_read, pinctrl_handler was not being woken up > (until another event wakes it). > This could cause icmp packets to be missing or delayed until the > pinctrl_handler thread is woken up by another event. > > Note that with this patch, pinctrl_handler might be woken up for nothing. > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > --- > controller/pinctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index eeb6f7527..a8a307a56 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -3460,6 +3460,7 @@ pinctrl_handler(void *arg_) > ovs_mutex_unlock(&pinctrl_mutex); > > rconn_run(swconn); > + new_seq = seq_read(pinctrl_handler_seq); > if (rconn_is_connected(swconn)) { > if (conn_seq_no != rconn_get_connection_seqno(swconn)) { > pinctrl_setup(swconn); > @@ -3506,7 +3507,6 @@ pinctrl_handler(void *arg_) > ipv6_prefixd_wait(send_prefixd_time); > bfd_monitor_wait(bfd_time); > > - new_seq = seq_read(pinctrl_handler_seq); > seq_wait(pinctrl_handler_seq, new_seq); > > latch_wait(&pctrl->pinctrl_thread_exit);
On 8/18/22 20:57, Mark Michelson wrote: > Hi Xavier. I think the only foolproof way to get the seq logic right in > pinctrl is to perform all of the processing of pinctrl_handler > (including reading the seq and waiting on the seq) with the > pinctrl_mutex locked. However, this could cause a lot of waiting that > we'd like to avoid. > > I think this change of reading the seq value early is the best > compromise. As you mentioned, we could end up getting woken up for > nothing, but it's better to be woken up for nothing than to be asleep > when we should be processing something. I agree. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Thanks for the patch, Xavier and for the review, Mark! Applied to main. Regards, Dumitru
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index eeb6f7527..a8a307a56 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -3460,6 +3460,7 @@ pinctrl_handler(void *arg_) ovs_mutex_unlock(&pinctrl_mutex); rconn_run(swconn); + new_seq = seq_read(pinctrl_handler_seq); if (rconn_is_connected(swconn)) { if (conn_seq_no != rconn_get_connection_seqno(swconn)) { pinctrl_setup(swconn); @@ -3506,7 +3507,6 @@ pinctrl_handler(void *arg_) ipv6_prefixd_wait(send_prefixd_time); bfd_monitor_wait(bfd_time); - new_seq = seq_read(pinctrl_handler_seq); seq_wait(pinctrl_handler_seq, new_seq); latch_wait(&pctrl->pinctrl_thread_exit);
seq_read should be used before processing any changes. If buffered_mac_bindings is updated by pinctrl_run while pinctrl_handler is running, but before seq_read, pinctrl_handler was not being woken up (until another event wakes it). This could cause icmp packets to be missing or delayed until the pinctrl_handler thread is woken up by another event. Note that with this patch, pinctrl_handler might be woken up for nothing. Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- controller/pinctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)