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;
> 

Reply via email to