tap: fix build of tap_bpf_program

  The tap_bpf_program.c is not built as part of normal DPDK
EAL environment. It is intended to be built standalone
and does not use rte_common.h.

This reverts the related change from
commit ef5baf3486e0 ("replace packed attributes")

Note: this patch will cause expected warnings from checkpatch
because the code involved is not used directly in DPDK environment.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/tap_bpf_program.c | 4 ++--
 drivers/net/tap/tap_rss.h         | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
  

On 7/17/2023 8:15 PM, Stephen Hemminger wrote:
> The tap_bpf_program.c is not built as part of normal DPDK
> EAL environment. It is intended to be built standalone
> and does not use rte_common.h.
> 
> This reverts the related change from
> commit ef5baf3486e0 ("replace packed attributes")
> 
> Note: this patch will cause expected warnings from checkpatch
> because the code involved is not used directly in DPDK environment.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>

Agree, this seems done by mistake as part of batch update,

Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>


But I can't update the bpf file at all, if I am not missing something I
am not sure if we should get just this update or have a patch/patchset
that fixes the build.

@Ophir, how the bpf file is compiled? And did you test it recently?

I am using command from the documentation:
`clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
-filetype=obj -o tap_bpf_program.o`



@Aaron, should we add a simple build test around it, since this file is
not build by default with DPDK build?


> ---
>  drivers/net/tap/tap_bpf_program.c | 4 ++--
>  drivers/net/tap/tap_rss.h         | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/tap/tap_bpf_program.c b/drivers/net/tap/tap_bpf_program.c
> index 20c310e5e7ba..532e8838fe27 100644
> --- a/drivers/net/tap/tap_bpf_program.c
> +++ b/drivers/net/tap/tap_bpf_program.c
> @@ -75,14 +75,14 @@ struct ipv4_l3_l4_tuple {
>  	__u32    dst_addr;
>  	__u16    dport;
>  	__u16    sport;
> -} __rte_packed;
> +} __attribute__((packed));
>  
>  struct ipv6_l3_l4_tuple {
>  	__u8        src_addr[16];
>  	__u8        dst_addr[16];
>  	__u16       dport;
>  	__u16       sport;
> -} __rte_packed;
> +} __attribute__((packed));
>  
>  static const __u8 def_rss_key[TAP_RSS_HASH_KEY_SIZE] = {
>  	0xd1, 0x81, 0xc6, 0x2c,
> diff --git a/drivers/net/tap/tap_rss.h b/drivers/net/tap/tap_rss.h
> index 48c151cf6b68..dff46a012f94 100644
> --- a/drivers/net/tap/tap_rss.h
> +++ b/drivers/net/tap/tap_rss.h
> @@ -35,6 +35,6 @@ struct rss_key {
>  	__u32 key_size;
>  	__u32 queues[TAP_MAX_QUEUES];
>  	__u32 nb_queues;
> -} __rte_packed;
> +} __attribute__((packed));
>  
>  #endif /* _TAP_RSS_H_ */
  

On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:
>> The tap_bpf_program.c is not built as part of normal DPDK
>> EAL environment. It is intended to be built standalone
>> and does not use rte_common.h.
>>
>> This reverts the related change from
>> commit ef5baf3486e0 ("replace packed attributes")
>>
>> Note: this patch will cause expected warnings from checkpatch
>> because the code involved is not used directly in DPDK environment.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>
> 
> Agree, this seems done by mistake as part of batch update,
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> 
> But I can't update the bpf file at all, if I am not missing something I

* I can't *compile* the bpf file ...

> am not sure if we should get just this update or have a patch/patchset
> that fixes the build.
> 
> @Ophir, how the bpf file is compiled? And did you test it recently?
> 
> I am using command from the documentation:
> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
> -filetype=obj -o tap_bpf_program.o`
> 
> 
> 
> @Aaron, should we add a simple build test around it, since this file is
> not build by default with DPDK build?
> 
> 
>> ---
>>  drivers/net/tap/tap_bpf_program.c | 4 ++--
>>  drivers/net/tap/tap_rss.h         | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/tap/tap_bpf_program.c b/drivers/net/tap/tap_bpf_program.c
>> index 20c310e5e7ba..532e8838fe27 100644
>> --- a/drivers/net/tap/tap_bpf_program.c
>> +++ b/drivers/net/tap/tap_bpf_program.c
>> @@ -75,14 +75,14 @@ struct ipv4_l3_l4_tuple {
>>  	__u32    dst_addr;
>>  	__u16    dport;
>>  	__u16    sport;
>> -} __rte_packed;
>> +} __attribute__((packed));
>>  
>>  struct ipv6_l3_l4_tuple {
>>  	__u8        src_addr[16];
>>  	__u8        dst_addr[16];
>>  	__u16       dport;
>>  	__u16       sport;
>> -} __rte_packed;
>> +} __attribute__((packed));
>>  
>>  static const __u8 def_rss_key[TAP_RSS_HASH_KEY_SIZE] = {
>>  	0xd1, 0x81, 0xc6, 0x2c,
>> diff --git a/drivers/net/tap/tap_rss.h b/drivers/net/tap/tap_rss.h
>> index 48c151cf6b68..dff46a012f94 100644
>> --- a/drivers/net/tap/tap_rss.h
>> +++ b/drivers/net/tap/tap_rss.h
>> @@ -35,6 +35,6 @@ struct rss_key {
>>  	__u32 key_size;
>>  	__u32 queues[TAP_MAX_QUEUES];
>>  	__u32 nb_queues;
>> -} __rte_packed;
>> +} __attribute__((packed));
>>  
>>  #endif /* _TAP_RSS_H_ */
>
  

On 7/19/2023 11:03 AM, Ferruh Yigit wrote:
> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
>> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:
>>> The tap_bpf_program.c is not built as part of normal DPDK
>>> EAL environment. It is intended to be built standalone
>>> and does not use rte_common.h.
>>>
>>> This reverts the related change from
>>> commit ef5baf3486e0 ("replace packed attributes")
>>>
>>> Note: this patch will cause expected warnings from checkpatch
>>> because the code involved is not used directly in DPDK environment.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>
>>
>> Agree, this seems done by mistake as part of batch update,
>>
>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>>
>>
>> But I can't update the bpf file at all, if I am not missing something I
> 
> * I can't *compile* the bpf file ...
> 
>> am not sure if we should get just this update or have a patch/patchset
>> that fixes the build.
>>
>> @Ophir, how the bpf file is compiled? And did you test it recently?
>>
>> I am using command from the documentation:
>> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
>> -filetype=obj -o tap_bpf_program.o`
>>
>>
>>
>> @Aaron, should we add a simple build test around it, since this file is
>> not build by default with DPDK build?
>>

I saw Stephen already touched to the topic:
https://inbox.dpdk.org/dev/20230717083032.072b12c5@hermes.local/


>>
>>> ---
>>>  drivers/net/tap/tap_bpf_program.c | 4 ++--
>>>  drivers/net/tap/tap_rss.h         | 2 +-
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/tap_bpf_program.c b/drivers/net/tap/tap_bpf_program.c
>>> index 20c310e5e7ba..532e8838fe27 100644
>>> --- a/drivers/net/tap/tap_bpf_program.c
>>> +++ b/drivers/net/tap/tap_bpf_program.c
>>> @@ -75,14 +75,14 @@ struct ipv4_l3_l4_tuple {
>>>  	__u32    dst_addr;
>>>  	__u16    dport;
>>>  	__u16    sport;
>>> -} __rte_packed;
>>> +} __attribute__((packed));
>>>  
>>>  struct ipv6_l3_l4_tuple {
>>>  	__u8        src_addr[16];
>>>  	__u8        dst_addr[16];
>>>  	__u16       dport;
>>>  	__u16       sport;
>>> -} __rte_packed;
>>> +} __attribute__((packed));
>>>  
>>>  static const __u8 def_rss_key[TAP_RSS_HASH_KEY_SIZE] = {
>>>  	0xd1, 0x81, 0xc6, 0x2c,
>>> diff --git a/drivers/net/tap/tap_rss.h b/drivers/net/tap/tap_rss.h
>>> index 48c151cf6b68..dff46a012f94 100644
>>> --- a/drivers/net/tap/tap_rss.h
>>> +++ b/drivers/net/tap/tap_rss.h
>>> @@ -35,6 +35,6 @@ struct rss_key {
>>>  	__u32 key_size;
>>>  	__u32 queues[TAP_MAX_QUEUES];
>>>  	__u32 nb_queues;
>>> -} __rte_packed;
>>> +} __attribute__((packed));
>>>  
>>>  #endif /* _TAP_RSS_H_ */
>>
>
  

On Wed, 19 Jul 2023 11:03:36 +0100
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
> > On 7/17/2023 8:15 PM, Stephen Hemminger wrote:  
> >> The tap_bpf_program.c is not built as part of normal DPDK
> >> EAL environment. It is intended to be built standalone
> >> and does not use rte_common.h.
> >>
> >> This reverts the related change from
> >> commit ef5baf3486e0 ("replace packed attributes")
> >>
> >> Note: this patch will cause expected warnings from checkpatch
> >> because the code involved is not used directly in DPDK environment.
> >>
> >> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>  
> > 
> > Agree, this seems done by mistake as part of batch update,
> > 
> > Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> > 
> > 
> > But I can't update the bpf file at all, if I am not missing something I  
> 
> * I can't *compile* the bpf file ...
> 
> > am not sure if we should get just this update or have a patch/patchset
> > that fixes the build.
> > 
> > @Ophir, how the bpf file is compiled? And did you test it recently?
> > 
> > I am using command from the documentation:
> > `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
> > -filetype=obj -o tap_bpf_program.o`

It looks like this won't work because it was expecting to be able
to find header files from older version of iproute2.  These are not
distributed, and the change to support libbpf in iproute2 makes the
current versions not work.

As a stopgap, will look back in history and see what version of header
files will at least get a working build.

From there, need to replace how the conversion of .o to array works.
Would prefer to use dlopen() to read the ELF file rather than expecting
developers to hack together their own tools.

Not sure how much effort is really needed here. This is only being
used for the case of rte_flow with multiq RSS. Probably, no one ever
used it.
  

On 7/19/2023 5:12 PM, Stephen Hemminger wrote:
> On Wed, 19 Jul 2023 11:03:36 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
>>> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:  
>>>> The tap_bpf_program.c is not built as part of normal DPDK
>>>> EAL environment. It is intended to be built standalone
>>>> and does not use rte_common.h.
>>>>
>>>> This reverts the related change from
>>>> commit ef5baf3486e0 ("replace packed attributes")
>>>>
>>>> Note: this patch will cause expected warnings from checkpatch
>>>> because the code involved is not used directly in DPDK environment.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>>  
>>>
>>> Agree, this seems done by mistake as part of batch update,
>>>
>>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>>>
>>>
>>> But I can't update the bpf file at all, if I am not missing something I  
>>
>> * I can't *compile* the bpf file ...
>>
>>> am not sure if we should get just this update or have a patch/patchset
>>> that fixes the build.
>>>
>>> @Ophir, how the bpf file is compiled? And did you test it recently?
>>>
>>> I am using command from the documentation:
>>> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
>>> -filetype=obj -o tap_bpf_program.o`
> 
> It looks like this won't work because it was expecting to be able
> to find header files from older version of iproute2.  These are not
> distributed, and the change to support libbpf in iproute2 makes the
> current versions not work.
> 
> As a stopgap, will look back in history and see what version of header
> files will at least get a working build.
> 
> From there, need to replace how the conversion of .o to array works.
> Would prefer to use dlopen() to read the ELF file rather than expecting
> developers to hack together their own tools.
> 
> Not sure how much effort is really needed here. This is only being
> used for the case of rte_flow with multiq RSS. Probably, no one ever
> used it.
>

Should we remove the file, instead of fixing '__rte_packed'?
  

On 7/20/2023 8:45 AM, Ferruh Yigit wrote:
> On 7/19/2023 5:12 PM, Stephen Hemminger wrote:
>> On Wed, 19 Jul 2023 11:03:36 +0100
>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>>> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
>>>> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:  
>>>>> The tap_bpf_program.c is not built as part of normal DPDK
>>>>> EAL environment. It is intended to be built standalone
>>>>> and does not use rte_common.h.
>>>>>
>>>>> This reverts the related change from
>>>>> commit ef5baf3486e0 ("replace packed attributes")
>>>>>
>>>>> Note: this patch will cause expected warnings from checkpatch
>>>>> because the code involved is not used directly in DPDK environment.
>>>>>
>>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>>>  
>>>>
>>>> Agree, this seems done by mistake as part of batch update,
>>>>
>>>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
>>>>
>>>>
>>>> But I can't update the bpf file at all, if I am not missing something I  
>>>
>>> * I can't *compile* the bpf file ...
>>>
>>>> am not sure if we should get just this update or have a patch/patchset
>>>> that fixes the build.
>>>>
>>>> @Ophir, how the bpf file is compiled? And did you test it recently?
>>>>
>>>> I am using command from the documentation:
>>>> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
>>>> -filetype=obj -o tap_bpf_program.o`
>>
>> It looks like this won't work because it was expecting to be able
>> to find header files from older version of iproute2.  These are not
>> distributed, and the change to support libbpf in iproute2 makes the
>> current versions not work.
>>
>> As a stopgap, will look back in history and see what version of header
>> files will at least get a working build.
>>
>> From there, need to replace how the conversion of .o to array works.
>> Would prefer to use dlopen() to read the ELF file rather than expecting
>> developers to hack together their own tools.
>>
>> Not sure how much effort is really needed here. This is only being
>> used for the case of rte_flow with multiq RSS. Probably, no one ever
>> used it.
>>
> 
> Should we remove the file, instead of fixing '__rte_packed'?
> 

+Long, and af_xdp maintainers,

@Long, do you know if this bfp code is still in use somewhere, if so is
the user interested in fixing/maintaining the code?


@Ciara, @Qi, do you see any benefit to keep/extend this kind of bfp file
usage? Do you think is this something to invest more?
  

> Subject: Re: [PATCH] tap: fix build of tap_bpf_program
> 
> On 7/20/2023 8:45 AM, Ferruh Yigit wrote:
> > On 7/19/2023 5:12 PM, Stephen Hemminger wrote:
> >> On Wed, 19 Jul 2023 11:03:36 +0100
> >> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >>> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
> >>>> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:
> >>>>> The tap_bpf_program.c is not built as part of normal DPDK EAL
> >>>>> environment. It is intended to be built standalone and does not
> >>>>> use rte_common.h.
> >>>>>
> >>>>> This reverts the related change from commit ef5baf3486e0 ("replace
> >>>>> packed attributes")
> >>>>>
> >>>>> Note: this patch will cause expected warnings from checkpatch
> >>>>> because the code involved is not used directly in DPDK environment.
> >>>>>
> >>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>>>
> >>>>
> >>>> Agree, this seems done by mistake as part of batch update,
> >>>>
> >>>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >>>>
> >>>>
> >>>> But I can't update the bpf file at all, if I am not missing
> >>>> something I
> >>>
> >>> * I can't *compile* the bpf file ...
> >>>
> >>>> am not sure if we should get just this update or have a
> >>>> patch/patchset that fixes the build.
> >>>>
> >>>> @Ophir, how the bpf file is compiled? And did you test it recently?
> >>>>
> >>>> I am using command from the documentation:
> >>>> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
> >>>> -filetype=obj -o tap_bpf_program.o`
> >>
> >> It looks like this won't work because it was expecting to be able to
> >> find header files from older version of iproute2.  These are not
> >> distributed, and the change to support libbpf in iproute2 makes the
> >> current versions not work.
> >>
> >> As a stopgap, will look back in history and see what version of
> >> header files will at least get a working build.
> >>
> >> From there, need to replace how the conversion of .o to array works.
> >> Would prefer to use dlopen() to read the ELF file rather than
> >> expecting developers to hack together their own tools.
> >>
> >> Not sure how much effort is really needed here. This is only being
> >> used for the case of rte_flow with multiq RSS. Probably, no one ever
> >> used it.
> >>
> >
> > Should we remove the file, instead of fixing '__rte_packed'?
> >
> 
> +Long, and af_xdp maintainers,
> 
> @Long, do you know if this bfp code is still in use somewhere, if so is the user
> interested in fixing/maintaining the code?

I haven't seen usage in the support cases we have handled. Given support cases represent very small percent of actual usage, I suspect it's still being used.
> 
> 
> @Ciara, @Qi, do you see any benefit to keep/extend this kind of bfp file usage?
> Do you think is this something to invest more?
  

> On 7/20/2023 8:45 AM, Ferruh Yigit wrote:
> > On 7/19/2023 5:12 PM, Stephen Hemminger wrote:
> >> On Wed, 19 Jul 2023 11:03:36 +0100
> >> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> >>
> >>> On 7/19/2023 11:00 AM, Ferruh Yigit wrote:
> >>>> On 7/17/2023 8:15 PM, Stephen Hemminger wrote:
> >>>>> The tap_bpf_program.c is not built as part of normal DPDK
> >>>>> EAL environment. It is intended to be built standalone
> >>>>> and does not use rte_common.h.
> >>>>>
> >>>>> This reverts the related change from
> >>>>> commit ef5baf3486e0 ("replace packed attributes")
> >>>>>
> >>>>> Note: this patch will cause expected warnings from checkpatch
> >>>>> because the code involved is not used directly in DPDK environment.
> >>>>>
> >>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> >>>>>
> >>>>
> >>>> Agree, this seems done by mistake as part of batch update,
> >>>>
> >>>> Acked-by: Ferruh Yigit <ferruh.yigit@amd.com>
> >>>>
> >>>>
> >>>> But I can't update the bpf file at all, if I am not missing something I
> >>>
> >>> * I can't *compile* the bpf file ...
> >>>
> >>>> am not sure if we should get just this update or have a patch/patchset
> >>>> that fixes the build.
> >>>>
> >>>> @Ophir, how the bpf file is compiled? And did you test it recently?
> >>>>
> >>>> I am using command from the documentation:
> >>>> `clang -O2 -emit-llvm -c tap_bpf_program.c -o - | llc -march=bpf
> >>>> -filetype=obj -o tap_bpf_program.o`
> >>
> >> It looks like this won't work because it was expecting to be able
> >> to find header files from older version of iproute2.  These are not
> >> distributed, and the change to support libbpf in iproute2 makes the
> >> current versions not work.
> >>
> >> As a stopgap, will look back in history and see what version of header
> >> files will at least get a working build.
> >>
> >> From there, need to replace how the conversion of .o to array works.
> >> Would prefer to use dlopen() to read the ELF file rather than expecting
> >> developers to hack together their own tools.
> >>
> >> Not sure how much effort is really needed here. This is only being
> >> used for the case of rte_flow with multiq RSS. Probably, no one ever
> >> used it.
> >>
> >
> > Should we remove the file, instead of fixing '__rte_packed'?
> >
> 
> +Long, and af_xdp maintainers,
> 
> @Long, do you know if this bfp code is still in use somewhere, if so is
> the user interested in fixing/maintaining the code?
> 
> 
> @Ciara, @Qi, do you see any benefit to keep/extend this kind of bfp file
> usage? Do you think is this something to invest more?

If the code is still being used I would agree with Stephen that using dlopen or libbpf to load the eBPF code would be preferable. The current steps are difficult to follow.