On 03/21/2019 03:14 AM, Paolo Abeni wrote:
> The queue is marked not empty after acquiring the seqlock,
> and it's up to the NOLOCK qdisc clearing such flag on dequeue.
> Since the empty status lays on the same cache-line of the
> seqlock, it's always hot on cache during the updates.
>
> This makes the empty flag update a little bit loosy. Given
> the lack of synchronization between enqueue and dequeue, this
> is unavoidable.
>
Seems nice !
> Signed-off-by: Paolo Abeni <pab...@redhat.com>
> ---
> include/net/sch_generic.h | 17 +++++++++++++++++
> net/sched/sch_generic.c | 2 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 31284c078d06..1dc0aeec5d39 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -113,6 +113,9 @@ struct Qdisc {
>
> spinlock_t busylock ____cacheline_aligned_in_smp;
> spinlock_t seqlock;
> +
> + /* for NOLOCK qdisc, true if there are enqueued skbs */
> + bool not_empty;
> struct rcu_head rcu;
> };
>
> @@ -143,11 +146,25 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc)
> return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
> }
>
> +static inline bool qdisc_is_empty(struct Qdisc *qdisc)
> +{
> + if (qdisc->flags & TCQ_F_NOLOCK)
> + return !qdisc->not_empty;
double negations are not very readable IMO ...
Why not using qdisc->empty instead ?
> + return !qdisc->q.qlen;
> +}
> +
> +/* for NOLOCK qdisc, caller must held seqlock */
> +static inline void qdisc_set_empty(struct Qdisc *qdisc)
> +{
> + qdisc->not_empty = false;
> +}
> +
I guess the helper is not really needed ?
> static inline bool qdisc_run_begin(struct Qdisc *qdisc)
> {
> if (qdisc->flags & TCQ_F_NOLOCK) {
> if (!spin_trylock(&qdisc->seqlock))
> return false;
> + qdisc->not_empty = true;
Not clear why you have a qdisc_set_empty() helper only for one case.
> } else if (qdisc_is_running(qdisc)) {
> return false;
> }
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a117d9260558..3b03c6b9be1e 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -671,6 +671,8 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc
> *qdisc)
> qdisc_qstats_cpu_backlog_dec(qdisc, skb);
> qdisc_bstats_cpu_update(qdisc, skb);
> qdisc_qstats_atomic_qlen_dec(qdisc);
> + } else {
> + qdisc_set_empty(qdisc);
> }
>
> return skb;
>
