Re: [PATCH net-next v4] xen networking: add basic XDP support for xen-netfront

[Denis Kirjanov <kda@linux-powerpc.org>]

On 3/30/20, Jürgen Groß <jgross@suse.com> wrote:
> On 30.03.20 15:09, Denis Kirjanov wrote:
>> On 3/30/20, Jürgen Groß <jgross@suse.com> wrote:
>>> On 30.03.20 14:16, Denis Kirjanov wrote:
>>>> On 3/23/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>> On 23.03.20 11:49, Denis Kirjanov wrote:
>>>>>> On 3/23/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>>>> On 23.03.20 11:15, Denis Kirjanov wrote:
>>>>>>>> On 3/18/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>>>>>> On 18.03.20 13:50, Denis Kirjanov wrote:
>>>>>>>>>> On 3/18/20, Jürgen Groß <jgross@suse.com> wrote:
>>>>>>>>>>> On 16.03.20 14:09, Denis Kirjanov wrote:
>>>>>>>>>>>> The patch adds a basic XDP processing to xen-netfront driver.
>>>>>>>>>>>>
>>>>>>>>>>>> We ran an XDP program for an RX response received from netback
>>>>>>>>>>>> driver. Also we request xen-netback to adjust data offset for
>>>>>>>>>>>> bpf_xdp_adjust_head() header space for custom headers.
>>>>>>>>>>>
>>>>>>>>>>> This is in no way a "verbose patch descriprion".
>>>>>>>>>>>
>>>>>>>>>>> I'm missing:
>>>>>>>>>>>
>>>>>>>>>>> - Why are you doing this. "Add XDP support" is not enough, for
>>>>>>>>>>> such
>>>>>>>>>>>         a change I'd like to see some performance numbers to get
>>>>>>>>>>> an
>>>>>>>>>>> idea
>>>>>>>>>>>         of the improvement to expect, or which additional
>>>>>>>>>>> functionality
>>>>>>>>>>>         for the user is available.
>>>>>>>>>> Ok, I'll try to measure  some numbers.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> - A short description for me as a Xen maintainer with only basic
>>>>>>>>>>>         networking know-how, what XDP programs are about (a link
>>>>>>>>>>> to
>>>>>>>>>>> some
>>>>>>>>>>>         more detailed doc is enough, of course) and how the
>>>>>>>>>>> interface
>>>>>>>>>>>         is working (especially for switching between XDP mode
>>>>>>>>>>> and
>>>>>>>>>>> normal
>>>>>>>>>>>         SKB processing).
>>>>>>>>>>
>>>>>>>>>> You can search for the "A practical introduction to XDP"
>>>>>>>>>> tutorial.
>>>>>>>>>> Actually there is a lot of information available regarding XDP,
>>>>>>>>>> you
>>>>>>>>>> can easily find it.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> - A proper description of the netfront/netback communication
>>>>>>>>>>> when
>>>>>>>>>>>         enabling or disabling XDP mode (who is doing what, is
>>>>>>>>>>> silencing
>>>>>>>>>>>         of the virtual adapter required, ...).
>>>>>>>>>> Currently we need only a header offset from netback driver so
>>>>>>>>>> that
>>>>>>>>>> we
>>>>>>>>>> can
>>>>>>>>>> put
>>>>>>>>>> custom encapsulation header if required and that's done using xen
>>>>>>>>>> bus
>>>>>>>>>> state switching,
>>>>>>>>>> so that:
>>>>>>>>>> - netback tells that it can adjust the header offset
>>>>>>>>>> - netfront part reads it
>>>>>>>>>
>>>>>>>>> Yes, but how is this synchronized with currently running network
>>>>>>>>> load?
>>>>>>>>> Assume you are starting without XDP being active and then you are
>>>>>>>>> activating it. How is the synchronization done from which request
>>>>>>>>> on
>>>>>>>>> the XDP headroom is available?
>>>>>>>>
>>>>>>>> Hi Jurgen,
>>>>>>>>
>>>>>>>> basically XDP is activated when you've assigned an xdp program to
>>>>>>>> the
>>>>>>>> networking device.
>>>>>>>> Assigning an xdp program means that we have to adjust a pointer
>>>>>>>> which
>>>>>>>> is RCU protected.
>>>>>>>
>>>>>>> This doesn't answer my question.
>>>>>>>
>>>>>>> You have basically two communication channels: the state of the
>>>>>>> frontend
>>>>>>> and backend for activation/deactivation of XDP, and the ring pages
>>>>>>> with
>>>>>>> the rx and tx requests and responses. How is the synchronization
>>>>>>> between
>>>>>>> those two channels done? So how does the other side know which of
>>>>>>> the
>>>>>>> packets in flight will then have XDP on or off?
>>>>>>
>>>>>> Right,
>>>>>> that's done in xen-netback using xenbus state:
>>>>>> - in xennet_xdp_set() we call xenbus_switch_state to tell xen-netback
>>>>>> to
>>>>>> adjust offset for an RX response.
>>>>>> -xen-netback reads the value from xenstore and adjusts the offset for
>>>>>> xen-netback
>>>>>> in xenvif_rx_data_slot() using vif->xdp_enabled flag.
>>>>>
>>>>> And before that all in-flight requests in the ring pages are being
>>>>> processed and no new requests are guaranteed to be enqueued?
>>>>
>>>> Actually I don't see the need to sync these requests since that all we
>>>> have to do is to copy
>>>> data with specified offset:
>>>> with xdp->enabled=1: copy with the offset XDP_PACKET_HEADROOM
>>>> with xdd->enabled=0: copy without the offset
>>>
>>> Isn't that racy?
>>>
>>> In xennet_xdp_set() you set queue->xdp_prog and then you change the
>>> state to Reconfiguring. From the time queue->xdp_prog is set you'll
>>> do the Xdp processing in xennet_get_responses(), even if the response
>>> you are working on doesn't have the headroom you need, as the backend
>>> didn't create it with headroom (it needs some time until it has seen
>>> the new state and could react on it by sending _new_ responses with
>>> headroom).
>>
>> Ah, I see. You mean that we have to wait until XenbusStateReconfigured
>> is set in
>> xen-netfront and only after that it's safe to process packets.
>
> Right. That is the problem of using a different communication channel
> for enabling/disabling XDP.

Ok, I'll update my patch.

Thanks for review!

>
>
> Juergen
>