On 11/14/2017 04:41 PM, Willem de Bruijn wrote:
>> /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
>> static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
>> {
>> - struct sk_buff *skb = sch->gso_skb;
>> + struct sk_buff *skb = skb_peek(&sch->gso_skb);
>>
>> if (skb) {
>> - sch->gso_skb = NULL;
>> + skb = __skb_dequeue(&sch->gso_skb);
>> qdisc_qstats_backlog_dec(sch, skb);
>> sch->q.qlen--;
>
> In lockless qdiscs, can this race, so that __skb_dequeue returns NULL?
> Same for its use in qdisc_peek_dequeued.
>
Yes, agree if this was used in lockless qdisc it could race. However,
I don't think it is actually used in the lockless cases yet. For pfifo
fast __skb_array_peek is used.
>> -static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>> +static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>> {
>
> Perhaps dev_requeue_skb_qdisc_locked is more descriptive. Or
> adding a lockdep_is_held(..) also documents that the __locked variant
> below is not just a lock/unlock wrapper around this inner function.
>
Adding lockdep annotation here makes sense to me.
