David Marchand <david.march...@redhat.com> 于2022年4月12日周二 16:40写道：
> On Thu, Apr 7, 2022 at 4:13 AM Peng He <xnhp0...@gmail.com> wrote:
> >
> > I understand there is no need to introduce a rcu api,
> > but at least put these codes into a separate function ? :-)
>
> No strong opinion.
>
>
> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> index 6601f2346..402cdcdf5 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -1805,6 +1805,7 @@ destruct(struct ofproto *ofproto_, bool del)
> >> struct rule_dpif *rule;
> >> struct oftable *table;
> >> struct ovs_list ams;
> >> + struct seq *seq;
> >>
> >> ofproto->backer->need_revalidate = REV_RECONFIGURE;
> >> xlate_txn_start();
> >> @@ -1848,6 +1849,14 @@ destruct(struct ofproto *ofproto_, bool del)
> >>
> >> seq_destroy(ofproto->ams_seq);
> >>
> >> + /* Wait for all the meter rules to be destroyed. */
> >> + seq = seq_create();
> >> + ovsrcu_synchronize();
> >> + seq_wait(seq, seq_read(seq));
> >> + ovsrcu_postpone__((void (*)(void *))seq_change, seq);
> >>
> >> + poll_block();
> >
> >
> > after poll block, we should check if seq is changed or not, other fds
> will
> > wake up block too. so we need a while loop to do the seq_wait + postpone
> + check
> > stuff.
> >
> >> + seq_destroy(seq);
> >> +
>
> If there can be other event nodes registered at this point, once you
> leave poll_block() no event node is kept.
>
IMHO, this is how OVS event loop works.
even you lose all other event nodes after the first poll_block returns,
eventually (after you got seq changes) you are not blocking and running to
the next
poll_block point, and every modules in between will check fd in its XX_run
function,
if event is ready, do the work, when EAGAIN happens, the node is added back.
all the previous lost events will be added back eventually.
So it's ok to call poll_block anywhere. I think.
Adding to this, poll_block -> timepoll -> poll can fail (though I am
> not clear it is something that is supposed to happen).
why fail？
Once a first poll_block() returns, the only event that this code can
> wait for is the seq changing.
> There should be no need to wake up with a timeout, since there is no
> useful work to do.
>
> Am I missing something else?
>
yes, timeout is for the RCU api, if used here, I agree no need to add
timeout.
>
>
> In the end, that would translate to:
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6601f23464..dd1b9e5138 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1805,6 +1805,8 @@ destruct(struct ofproto *ofproto_, bool del)
> struct rule_dpif *rule;
> struct oftable *table;
> struct ovs_list ams;
> + struct seq *seq;
> + uint64_t seqno;
>
> ofproto->backer->need_revalidate = REV_RECONFIGURE;
> xlate_txn_start();
> @@ -1848,6 +1850,17 @@ destruct(struct ofproto *ofproto_, bool del)
>
> seq_destroy(ofproto->ams_seq);
>
> + /* Wait for all the meter rules to be destroyed. */
> + seq = seq_create();
> + seqno = seq_read();
> + ovsrcu_synchronize();
> + ovsrcu_postpone__((void (*)(void *))seq_change, seq);
> + do {
> + seq_wait(seq, seqno);
> + poll_block();
> + } while (seqno == seq_read(seq));
> + seq_destroy(seq);
> +
> close_dpif_backer(ofproto->backer, del);
> }
>
>
> --
> David Marchand
>
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
