On 12 Apr 2022, at 11:09, Peng He wrote:
> 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.
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.
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.
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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
