Re: [PATCH net-next v2] xen-netfront: add basic XDP support

["Jürgen Groß" <jgross@suse.com>]

On 05.03.20 10:47, Denis Kirjanov wrote:
> On 3/4/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 04.03.20 14:10, Denis Kirjanov wrote:
>>> On 3/2/20, Jürgen Groß <jgross@suse.com> wrote:
>>>> On 02.03.20 15:21, Denis Kirjanov wrote:
>>>>> the patch adds a basic xdo logic to the netfront driver
>>>>>
>>>>> XDP redirect is not supported yet
>>>>>
>>>>> v2:
>>>>> - avoid data copying while passing to XDP
>>>>> - tell xen-natback that we need the headroom space
>>>>
>>>> Please add the patch history below the "---" delimiter
>>>>
>>>>>
>>>>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>>>>> ---
>>>>>     drivers/net/xen-netback/common.h |   1 +
>>>>>     drivers/net/xen-netback/rx.c     |   9 ++-
>>>>>     drivers/net/xen-netback/xenbus.c |  21 ++++++
>>>>>     drivers/net/xen-netfront.c       | 157
>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>     4 files changed, 186 insertions(+), 2 deletions(-)
>>>>
>>>> You are modifying xen-netback sources. Please Cc the maintainers.
>>>>
>>
>> ...
>>
>>>>>
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +abort_transaction:
>>>>> +	xenbus_dev_fatal(np->xbdev, err, "%s", message);
>>>>> +	xenbus_transaction_end(xbt, 1);
>>>>> +out:
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>> +static int xennet_xdp_set(struct net_device *dev, struct bpf_prog
>>>>> *prog,
>>>>> +			struct netlink_ext_ack *extack)
>>>>> +{
>>>>> +	struct netfront_info *np = netdev_priv(dev);
>>>>> +	struct bpf_prog *old_prog;
>>>>> +	unsigned int i, err;
>>>>> +
>>>>> +	old_prog = rtnl_dereference(np->queues[0].xdp_prog);
>>>>> +	if (!old_prog && !prog)
>>>>> +		return 0;
>>>>> +
>>>>> +	if (prog)
>>>>> +		bpf_prog_add(prog, dev->real_num_tx_queues);
>>>>> +
>>>>> +	for (i = 0; i < dev->real_num_tx_queues; ++i)
>>>>> +		rcu_assign_pointer(np->queues[i].xdp_prog, prog);
>>>>> +
>>>>> +	if (old_prog)
>>>>> +		for (i = 0; i < dev->real_num_tx_queues; ++i)
>>>>> +			bpf_prog_put(old_prog);
>>>>> +
>>>>> +	err = talk_to_netback_xdp(np, old_prog ?
>>>>> NETBACK_XDP_HEADROOM_DISABLE:
>>>>> +				  NETBACK_XDP_HEADROOM_ENABLE);
>>>>> +	if (err)
>>>>> +		return err;
>>>>> +
>>>>> +	xenbus_switch_state(np->xbdev, XenbusStateReconfiguring);
>>>>
>>>> What is happening in case the backend doesn't support XDP?
>>> Here we just ask xen-backend to make a headroom, that's it.
>>> It's better to send xen-backend changes in a separate patch.
>>
>> Okay, but what do you do if the backend doesn't support XDP (e.g. in
>> case its an older kernel)? How do you know it is supporting XDP?
> We can check a xenbus reply to xenbus state change.

Using the frontend state for that purpose is rather dangerous.

In case the backend doesn't support the "Reconfiguring" state you might
end up with a broken network.

I'd rather let the backend advertise the support via a "feature-xdp"
node in Xenstore and do the activation via another node.


Juergen