> On 19 May 2023, at 19:35, Claudio Jeker <cje...@diehard.n-r-g.com> wrote:
>
> On Fri, May 19, 2023 at 06:10:19PM +1000, David Gwynne wrote:
>> On Fri, May 19, 2023 at 08:11:13AM +0200, Claudio Jeker wrote:
>>> On Fri, May 19, 2023 at 01:56:38PM +1000, David Gwynne wrote:
>>>> this is a tiny slice off a big pfsync diff i've been working on. when
>>>> you bring pfsync down i need it to wait until all the work it's been
>>>> doing in the network stack has finished, which means i need a barrier
>>>> for all the network taskqs. that's what this implements.
>>>>
>>>> a barrier per taskq would mean iterating over the taskqs and waiting for
>>>> a barrier on each one. by using a refcnt and shoving a task onto each of
>>>> them in one go, i only have to wait for the slowest one, not all
>>>> of them in series.
>>>>
>>>> ok?
>>>
>>> Not sure if it matters here but wouldn't it be even better to insert this
>>> barrier on the head of the task queue? At least I think you just want one
>>> task run to be done.
>>
>> that suggestion is a bit triggering for me because:
>>
>> https://cvsweb.openbsd.org/src/sys/kern/kern_task.c#rev1.29
>
> Oh my. What a night mare. One could implement flush_workqueue() via
> taskq_add() if we ever want to make taskq_barrier faster.
>
>>> Now 'ifconfig pfsync0 down' is not a hot path so it does not matter but
>>> such barriers have the tendency to end up in unexpected places.
>>>
>>> Running all tasks in parallel is a good compromise right now.
>>
>> i'd still run the tasks in parallel, even if i queued the barrier work
>> at the head of the taskq.
>
> Yes, I did not formulate that well. I think running the barrier in parallel
> is a good idea and better than what is done in some of other barriers.
>
> I got burned by the smr_barrier and the fact that this barrier can take
> quite long to finish. Because of that it is not that trivial to use SMR
> in latency critical code.
the ratio of smr pointer reads to updates is a little different in this
situation...
>
>>> OK claudio@
>>>
>>>> Index: if.c
>>>> ===================================================================
>>>> RCS file: /cvs/src/sys/net/if.c,v
>>>> retrieving revision 1.696
>>>> diff -u -p -r1.696 if.c
>>>> --- if.c 14 May 2023 01:46:53 -0000 1.696
>>>> +++ if.c 19 May 2023 03:50:10 -0000
>>>> @@ -3481,3 +3481,19 @@ net_tq(unsigned int ifindex)
>>>>
>>>> return (sn->sn_taskq);
>>>> }
>>>> +
>>>> +void
>>>> +net_tq_barriers(const char *wmesg)
>>>> +{
>>>> + struct task barriers[NET_TASKQ];
>>>> + struct refcnt r = REFCNT_INITIALIZER();
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < nitems(barriers); i++) {
>>>> + task_set(&barriers[i], (void (*)(void *))refcnt_rele_wake, &r);
>>>> + refcnt_take(&r);
>>>> + task_add(softnets[i].sn_taskq, &barriers[i]);
>>>> + }
>>>> +
>>>> + refcnt_finalize(&r, wmesg);
>>>> +}
>>>> Index: if.h
>>>> ===================================================================
>>>> RCS file: /cvs/src/sys/net/if.h,v
>>>> retrieving revision 1.212
>>>> diff -u -p -r1.212 if.h
>>>> --- if.h 15 May 2023 16:34:56 -0000 1.212
>>>> +++ if.h 19 May 2023 03:50:10 -0000
>>>> @@ -560,6 +560,7 @@ int if_congested(void);
>>>> __dead void unhandled_af(int);
>>>> int if_setlladdr(struct ifnet *, const uint8_t *);
>>>> struct taskq * net_tq(unsigned int);
>>>> +void net_tq_barriers(const char *);
>>>>
>>>> #endif /* _KERNEL */
>>>>
>>>>
>>>
>>> --
>>> :wq Claudio
>>
>
> --
> :wq Claudio
