Re: Packet gets stuck in NOLOCK pfifo_fast qdisc

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2021/4/6 9:49, Cong Wang wrote:
> On Sat, Apr 3, 2021 at 5:23 AM Jiri Kosina <jikos@kernel.org> wrote:
>>
>> I am still planning to have Yunsheng Lin's (CCing) fix [1] tested in the
>> coming days. If it works, then we can consider proceeding with it,
>> otherwise I am all for reverting the whole NOLOCK stuff.
>>
>> [1] https://lore.kernel.org/linux-can/1616641991-14847-1-git-send-email-linyunsheng@huawei.com/T/#u
> 
> I personally prefer to just revert that bit, as it brings more troubles
> than gains. Even with Yunsheng's patch, there are still some issues.
> Essentially, I think the core qdisc scheduling code is not ready for
> lockless, just look at those NOLOCK checks in sch_generic.c. :-/

I am also awared of the NOLOCK checks too:), and I am willing to
take care of it if that is possible.

As the number of cores in a system is increasing, it is the trend
to become lockless, right? Even there is only one cpu involved, the
spinlock taking and releasing takes about 30ns on our arm64 system
when CONFIG_PREEMPT_VOLUNTARY is enable(ip forwarding testing).

Currently I has three ideas to optimize the lockless qdisc:
1. implement the qdisc bypass for lockless qdisc too, see [1].

2. implement lockless enqueuing for lockless qdisc using the idea
   from Jason and Toke. And it has a noticable proformance increase with
   1-4 threads running using the below prototype based on ptr_ring.

static inline int __ptr_ring_multi_produce(struct ptr_ring *r, void *ptr)
{

        int producer, next_producer;


        do {
                producer = READ_ONCE(r->producer);
                if (unlikely(!r->size) || r->queue[producer])
                        return -ENOSPC;
                next_producer = producer + 1;
                if (unlikely(next_producer >= r->size))
                        next_producer = 0;
        } while(cmpxchg_relaxed(&r->producer, producer, next_producer) != producer);

        /* Make sure the pointer we are storing points to a valid data. */
        /* Pairs with the dependency ordering in __ptr_ring_consume. */
        smp_wmb();

        WRITE_ONCE(r->queue[producer], ptr);
        return 0;
}

3. Maybe it is possible to remove the netif_tx_lock for lockless qdisc
   too, because dev_hard_start_xmit is also in the protection of
   qdisc_run_begin()/qdisc_run_end()(if there is only one qdisc using
   a netdev queue, which is true for pfifo_fast, I believe).


[1]. https://patchwork.kernel.org/project/netdevbpf/patch/1616404156-11772-1-git-send-email-linyunsheng@huawei.com/

> 
> Thanks.
> 
> .
>