[PATCH] public/io/netif.h: add a new extra type for XDP

[PATCH] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

The patch adds a new extra type to be able to diffirentiate
between RX responses on xen-netfront side with the adjusted offset
required for XDP processing.

For Linux the offset value is going to be passed via xenstore.

Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
---
 xen/include/public/io/netif.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 9fcf91a..759c88a 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -985,7 +985,8 @@ typedef struct netif_tx_request netif_tx_request_t;
 #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
 #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
 #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
-#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
+#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
+#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
 
 /* netif_extra_info_t flags. */
 #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
@@ -1018,6 +1019,10 @@ struct netif_extra_info {
             uint8_t algorithm;
             uint8_t value[4];
         } hash;
+        struct {
+            uint16_t headroom;
+            uint32_t pad;
+        } xdp;
         uint16_t pad[3];
     } u;
 };
-- 
1.8.3.1

Re: [PATCH] public/io/netif.h: add a new extra type for XDP

[Jürgen Groß]

On 18.05.20 10:24, Denis Kirjanov wrote:
> The patch adds a new extra type to be able to diffirentiate
> between RX responses on xen-netfront side with the adjusted offset
> required for XDP processing.
> 
> For Linux the offset value is going to be passed via xenstore.

Why? I can only see disadvantages by using a different communication
mechanism.

> 
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
>   xen/include/public/io/netif.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 9fcf91a..759c88a 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -985,7 +985,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>   #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>   #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>   #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>   
>   /* netif_extra_info_t flags. */
>   #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
> @@ -1018,6 +1019,10 @@ struct netif_extra_info {
>               uint8_t algorithm;
>               uint8_t value[4];
>           } hash;
> +        struct {
> +            uint16_t headroom;
> +            uint32_t pad;

Please use uint16_t pad[2] in order to avoid padding holes.

Additionally you are missing the addition of the related feature
Xenstore nodes in the comment area further up in the same file.


Juergen

Re: [PATCH] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
> On 18.05.20 10:24, Denis Kirjanov wrote:
>> The patch adds a new extra type to be able to diffirentiate
>> between RX responses on xen-netfront side with the adjusted offset
>> required for XDP processing.
>>
>> For Linux the offset value is going to be passed via xenstore.
>
> Why? I can only see disadvantages by using a different communication
> mechanism.
I see it like other features passed through xenstore and it requires
less changes to
other structures with the same final result.

>
>>
>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> ---
>>   xen/include/public/io/netif.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/io/netif.h
>> b/xen/include/public/io/netif.h
>> index 9fcf91a..759c88a 100644
>> --- a/xen/include/public/io/netif.h
>> +++ b/xen/include/public/io/netif.h
>> @@ -985,7 +985,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>>   #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>>   #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>>   #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
>> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
>> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
>> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>>
>>   /* netif_extra_info_t flags. */
>>   #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
>> @@ -1018,6 +1019,10 @@ struct netif_extra_info {
>>               uint8_t algorithm;
>>               uint8_t value[4];
>>           } hash;
>> +        struct {
>> +            uint16_t headroom;
>> +            uint32_t pad;
>
> Please use uint16_t pad[2] in order to avoid padding holes.
>
> Additionally you are missing the addition of the related feature
> Xenstore nodes in the comment area further up in the same file.

Done.

>
>
> Juergen
>

Re: [PATCH] public/io/netif.h: add a new extra type for XDP

[Jürgen Groß]

On 18.05.20 11:52, Denis Kirjanov wrote:
> On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 18.05.20 10:24, Denis Kirjanov wrote:
>>> The patch adds a new extra type to be able to diffirentiate
>>> between RX responses on xen-netfront side with the adjusted offset
>>> required for XDP processing.
>>>
>>> For Linux the offset value is going to be passed via xenstore.
>>
>> Why? I can only see disadvantages by using a different communication
>> mechanism.
> I see it like other features passed through xenstore and it requires
> less changes to
> other structures with the same final result.

This is okay as long there is no Xenstore interaction required when the
interface has been setup completely (i.e. only defining the needed
offset for XDP is fine, enabling/disabling XDP at runtime should not be
done via Xenstore IMO).

And please, no guest type special casing. Please replace "Linux" by e.g.
"The guest" (with additional tweaking of the following sentence).


Juergen

Re: [PATCH] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
> On 18.05.20 11:52, Denis Kirjanov wrote:
>> On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
>>> On 18.05.20 10:24, Denis Kirjanov wrote:
>>>> The patch adds a new extra type to be able to diffirentiate
>>>> between RX responses on xen-netfront side with the adjusted offset
>>>> required for XDP processing.
>>>>
>>>> For Linux the offset value is going to be passed via xenstore.
>>>
>>> Why? I can only see disadvantages by using a different communication
>>> mechanism.
>> I see it like other features passed through xenstore and it requires
>> less changes to
>> other structures with the same final result.
>
> This is okay as long there is no Xenstore interaction required when the
> interface has been setup completely (i.e. only defining the needed
> offset for XDP is fine, enabling/disabling XDP at runtime should not be
> done via Xenstore IMO).
I've checked netfront-<--->netback interaction and found no problems with it.
Paul found an issue that the value of the netfront state hasn't been
re-read (during unbind-bind sequence in dom0) and I've fixed it the
patch for the guest

>
> And please, no guest type special casing. Please replace "Linux" by e.g.
> "The guest" (with additional tweaking of the following sentence).

Oh, just sent v2. I'll fix a comment.

>
>
> Juergen
>

Re: [PATCH] public/io/netif.h: add a new extra type for XDP

[Jürgen Groß]

On 18.05.20 12:37, Denis Kirjanov wrote:
> On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
>> On 18.05.20 11:52, Denis Kirjanov wrote:
>>> On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
>>>> On 18.05.20 10:24, Denis Kirjanov wrote:
>>>>> The patch adds a new extra type to be able to diffirentiate
>>>>> between RX responses on xen-netfront side with the adjusted offset
>>>>> required for XDP processing.
>>>>>
>>>>> For Linux the offset value is going to be passed via xenstore.
>>>>
>>>> Why? I can only see disadvantages by using a different communication
>>>> mechanism.
>>> I see it like other features passed through xenstore and it requires
>>> less changes to
>>> other structures with the same final result.
>>
>> This is okay as long there is no Xenstore interaction required when the
>> interface has been setup completely (i.e. only defining the needed
>> offset for XDP is fine, enabling/disabling XDP at runtime should not be
>> done via Xenstore IMO).
> I've checked netfront-<--->netback interaction and found no problems with it.
> Paul found an issue that the value of the netfront state hasn't been
> re-read (during unbind-bind sequence in dom0) and I've fixed it the
> patch for the guest

I don't say your variant isn't working, but a feature being switchable
dynamically AND needing a ring page synchronization anyway should IMO
be switched via a specific ring page request.

I might have not the complete picture, so in case you have a good reason
to do it via Xenstore, fine, but "I'm seeing no problem with it" is no
good reason for a specific design decision.

> 
>>
>> And please, no guest type special casing. Please replace "Linux" by e.g.
>> "The guest" (with additional tweaking of the following sentence).
> 
> Oh, just sent v2. I'll fix a comment.

Thanks.


Juergen

RE: [PATCH] public/io/netif.h: add a new extra type for XDP

[Paul Durrant]

> -----Original Message-----
> From: Jürgen Groß <jgross@suse.com>
> Sent: 18 May 2020 11:28
> To: Denis Kirjanov <kda@linux-powerpc.org>
> Cc: xen-devel@lists.xenproject.org; paul@xen.org
> Subject: Re: [PATCH] public/io/netif.h: add a new extra type for XDP
> 
> On 18.05.20 11:52, Denis Kirjanov wrote:
> > On 5/18/20, Jürgen Groß <jgross@suse.com> wrote:
> >> On 18.05.20 10:24, Denis Kirjanov wrote:
> >>> The patch adds a new extra type to be able to diffirentiate
> >>> between RX responses on xen-netfront side with the adjusted offset
> >>> required for XDP processing.
> >>>
> >>> For Linux the offset value is going to be passed via xenstore.
> >>
> >> Why? I can only see disadvantages by using a different communication
> >> mechanism.
> > I see it like other features passed through xenstore and it requires
> > less changes to
> > other structures with the same final result.
> 
> This is okay as long there is no Xenstore interaction required when the
> interface has been setup completely (i.e. only defining the needed
> offset for XDP is fine, enabling/disabling XDP at runtime should not be
> done via Xenstore IMO).

FWIW it is for this kind of thing that I introduced the control ring, but that may be overkill for this.

  Paul

> 
> And please, no guest type special casing. Please replace "Linux" by e.g.
> "The guest" (with additional tweaking of the following sentence).
> 
> 
> Juergen