Hi, Eelco Thanks for your comments. Eelco Chaudron <[email protected]> 于2022年4月29日周五 18:54写道：
> > > On 12 Apr 2022, at 11:09, Peng He wrote: > > > David Marchand <[email protected]> 于2022年4月12日周二 16:40写道： > > > >> On Thu, Apr 7, 2022 at 4:13 AM Peng He <[email protected]> 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. > > > Sorry for joining in late in the conversation, but I do not see any harm > in introducing the new rcu API. It will be way cleaner, and if we add some > good documentation (and maybe a checkpatch warning), I think we should be > ok. the checkpatch warning is about ? an experimental API for RCU? > However looking at the implementation from patch 1, I see two problems > (well they are is actually one): > > + > +static void > +ovsrcu_barrier_func(void *seq_) > +{ > + struct seq *seq = (struct seq*)seq_; > + seq_change(seq); > +} > + > +bool > +ovsrcu_barrier(struct seq *seq, long long int timeout) > +{ > + /* first let all threads flush their cbset */ > + ovsrcu_synchronize(); > + > + /* then register a new cbset, ensure this cbset > + * is at the tail of global listi > + */ > + uint64_t seqno = seq_read(seq); > + ovsrcu_postpone__(ovsrcu_barrier_func, (void*)seq); > + long long int now = time_msec(); > + long long int deadline = now + timeout; > + > + do { > + seq_wait(seq, seqno); > + poll_timer_wait_until(deadline); > + poll_block(); > + now = time_msec(); /* update now */ > + } while (seqno == seq_read(seq) && now < deadline); > + > + return !(seqno == seq_read(seq)); > +} > > 1) I do not like that you have to supply the seq as a parameter. The API > should not take any parameters if you ask me. > 2) The timeout is a problem here, if the timeout happens you will call > seq_destroy(), so the later invocation of ovsrcu_barrier_func() would > access invalid memory. > Agree, will send a new version. > > So I guess the API should look something like this (not tested, but also > changed comments ;): > > +void > +ovsrcu_barrier() > +{ > + struct seq *seq = seq_create(); > + > + /* First let all threads flush their cbset’s. */ > + ovsrcu_synchronize(); > + > + /* Then register a new cbset, ensure this cbset > + * is at the tail of the global list. > + */ > + uint64_t seqno = seq_read(seq); > + ovsrcu_postpone__(ovsrcu_barrier_func, (void*)seq); > + > + do { > + seq_wait(seq, seqno); > + poll_block(); > + } while (seqno == seq_read(seq)); > + > + seq_destroy(seq); > +} > > Which you can then just use as (see comment changes ;): > > + /* Wait for all the meter destroy work to finish. */ > + ovsrcu_barrier(); > close_dpif_backer(ofproto->backer, del); > } > > > >> > >> > >> 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 > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
