[ovs-dev] [PATCH 2/2] netdev-afxdp: Add need_wakeup supprt.

[William Tu]

On Wed, Sep 4, 2019 at 7:27 AM Ilya Maximets <i.maximets at samsung.com> wrote:
>
> On 04.09.2019 17:04, William Tu wrote:
> > Hi Ilya,
> >
> > Thanks for the feedback.
> >
> > On Wed, Sep 4, 2019 at 5:19 AM Ilya Maximets <i.maximets at samsung.com> wrote:
> >>
> >> Hi William,
> >>
> >> Thanks for the patch!
> >> One general comment is that we, probably, should make this feature
> >> configurable.  There are 2 reasons:
> >>
> >> 1. We might want to run OVS on older kernel while building with more
> >>    recent libbpf. In this case socket creation will fail due to
> >>    unsupported flag and we'll not be able to make it work.
> >
> > I think it will also work, it falls back to old behavior.
> > From:
> > commit 77cd0d7b3f257fd0e3096b4fdcff1a7d38e99e10
> > Author: Magnus Karlsson <magnus.karlsson at intel.com>
> > Date:   Wed Aug 14 09:27:17 2019 +0200
> > "
> >    This flag needs some simple driver support. If the driver does not
> >     support this, the Rx flag is always zero and the Tx flag is always
> >     one. This makes any application relying on this feature default to the
> >     old behaviour of not requiring any syscalls in the Rx path and always
> >     having to call sendto() in the Tx path.
> > "
>
> This part is about relation between xdp subsystem and the device driver.
> If device driver doesn't support this feature, xdp subsystem will handle
> this by always exposing need_wakeup flag on rings.
> However, if bind_flags contains XDP_USE_NEED_WAKEUP, but xdp subsystem
> doesn't support it, userspace/libbpf will get failure on socket creation.
> To be honest, I didn't test that, but I think that it works this way.
> You may try building OVS with libbpf from bpf-next and run it on master
> kernel (without need_wakeup support).

Yes, you're right, it will fail at xsk_bind when checking
    if (flags & ~(XDP_SHARED_UMEM | XDP_COPY | XDP_ZEROCOPY) )

>
> >
> >> 2. Performance impact is not always positive. It depends on the
> >>    number of available CPU cores, port types (phisical or virtual),
> >>    interrupts affinity.  And this was proved by test results from
> >>    Eelco.  So, it might be good to have control over the enabling/
> >>    disabling the feature.
> >>
> >> However, I think that it's better to keep it enabled by default.
> >
> > OK, I will make it enabled by default, and add another option.
> > "options:need_wakeup=false" for user to disable it.
>
> This might be better to call it 'options:use_need_wakeup' because
> 'need_wakeup=false' sounds like we don't need to ever wake it up,
> but it's opposite in practice.

OK
Thanks
William