Does ACL support field size of 8 bytes?

Does ACL support field size of 8 bytes?

[Ido Goshen]

Hi

I've lots of good experience with ACL but can't make it work with u64 values 
I know it can be split to 2xu32 fields, but it makes it more complex to use and a wastes double  number of fields (we hit the RTE_ACL_MAX_FIELDS 64 limit)

According to the documentation and rte_acl.h fields size can be 8 bytes (u64)
e.g.   
  'The size parameter defines the length of the field in bytes. Allowable values are 1, 2, 4, or 8 bytes.'  
  (from https://doc.dpdk.org/guides-21.11/prog_guide/packet_classif_access_ctrl.html#rule-definition)

Though there's a hint it's less recommended 
  'Also, it is best to define fields of 8 or more bytes as 4 byte fields so that the build processes can eliminate fields that are all wild.'

It's also not clear how it fits in a group (i.e. what's input_index stride) which is only 4 bytes
    'All subsequent fields has to be grouped into sets of 4 consecutive bytes.'

I couldn't find any example or test app that's using 8 bytes
e.g. for IPv6 address 4xu32 fields are always used and not 2xu64

Should it work? 
Did anyone try it successfully and/or can share an example?

Thanx,
- Ido

RE: Does ACL support field size of 8 bytes?

[Ananyev, Konstantin]

Hi Ido,

> I've lots of good experience with ACL but can't make it work with u64 values
> I know it can be split to 2xu32 fields, but it makes it more complex to use and a wastes double  number of fields (we hit the
> RTE_ACL_MAX_FIELDS 64 limit)

Wow, that's a lot of fields...

> According to the documentation and rte_acl.h fields size can be 8 bytes (u64)
> e.g.
>   'The size parameter defines the length of the field in bytes. Allowable values are 1, 2, 4, or 8 bytes.'
>   (from https://doc.dpdk.org/guides-21.11/prog_guide/packet_classif_access_ctrl.html#rule-definition)
> 
> Though there's a hint it's less recommended
>   'Also, it is best to define fields of 8 or more bytes as 4 byte fields so that the build processes can eliminate fields that are all wild.'
>
> It's also not clear how it fits in a group (i.e. what's input_index stride) which is only 4 bytes
>     'All subsequent fields has to be grouped into sets of 4 consecutive bytes.'
> 
> I couldn't find any example or test app that's using 8 bytes
> e.g. for IPv6 address 4xu32 fields are always used and not 2xu64
> 
> Should it work?
> Did anyone try it successfully and/or can share an example?

You are right: though it is formally supported, we do not test it, and AFAIK no-one used it till now.
As we do group fields by 4B long chunks anyway, 8B field is sort of awkward and confusing.
To be honest, I don't even remember what was the rationale beyond introducing it at first place.
Anyway, just submitted patches that should fix 8B field support (at least it works for me now):
https://patches.dpdk.org/project/dpdk/list/?series=22676
Please give it a try.
In long term it probably would be good to hear from you and other users, should we keep 8B
support at all, or might be it would be easier just to abandon it.
Thanks
Konstantin 


 

Fwd: Does ACL support field size of 8 bytes?

[Konstantin Ananyev]

Hi Ido,

> I've lots of good experience with ACL but can't make it work with u64 values
> I know it can be split to 2xu32 fields, but it makes it more complex to use and a wastes double  number of fields (we hit the
> RTE_ACL_MAX_FIELDS 64 limit)

Wow, that's a lot of fields...

> According to the documentation and rte_acl.h fields size can be 8 bytes (u64)
> e.g.
>   'The size parameter defines the length of the field in bytes. Allowable values are 1, 2, 4, or 8 bytes.'
>   (from https://doc.dpdk.org/guides-21.11/prog_guide/packet_classif_access_ctrl.html#rule-definition)
> 
> Though there's a hint it's less recommended
>   'Also, it is best to define fields of 8 or more bytes as 4 byte fields so that the build processes can eliminate fields that are all wild.'
>
> It's also not clear how it fits in a group (i.e. what's input_index stride) which is only 4 bytes
>     'All subsequent fields has to be grouped into sets of 4 consecutive bytes.'
> 
> I couldn't find any example or test app that's using 8 bytes
> e.g. for IPv6 address 4xu32 fields are always used and not 2xu64
> 
> Should it work?
> Did anyone try it successfully and/or can share an example?

You are right: though it is formally supported, we do not test it, and 
AFAIK no-one used it till now.
As we do group fields by 4B long chunks anyway, 8B field is sort of 
awkward and confusing.
To be honest, I don't even remember what was the rationale beyond 
introducing it at first place.
Anyway, just submitted patches that should fix 8B field support (at 
least it works for me now):
https://patches.dpdk.org/project/dpdk/list/?series=22676
Please give it a try.
In long term it probably would be good to hear from you and other users, 
should we keep 8B
support at all, or might be it would be easier just to abandon it.
Thanks
Konstantin

RE: Does ACL support field size of 8 bytes?

[Ido Goshen]

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, 26 April 2022 20:57
> To: Ido Goshen <Ido@cgstowernetworks.com>; users@dpdk.org;
> dev@dpdk.org
> Cc: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
> Subject: RE: Does ACL support field size of 8 bytes?
> 
> 
> Hi Ido,
> 
> > I've lots of good experience with ACL but can't make it work with u64
> > values I know it can be split to 2xu32 fields, but it makes it more
> > complex to use and a wastes double  number of fields (we hit the
> > RTE_ACL_MAX_FIELDS 64 limit)
> 
> Wow, that's a lot of fields...

[idog] 
We provide a general purpose packet-broker that covers wide range of 
l2-l4 protocols + tunnels + some app level metadata. 
Though in most cases they won't be used simultaneously and many fields 
may end up being don't-care (e.g. mask=0) it's easier to code a static 
rte_acl_field_def struct that covers all the options then constructing it 
dynamically on each user configuration change
 
> > According to the documentation and rte_acl.h fields size can be 8[idog] 
> > ...
> > Should it work?
> > Did anyone try it successfully and/or can share an example?
> 
> You are right: though it is formally supported, we do not test it, and AFAIK no-
> one used it till now.
> As we do group fields by 4B long chunks anyway, 8B field is sort of awkward and
> confusing.
> To be honest, I don't even remember what was the rationale beyond introducing
> it at first place.
> Anyway, just submitted patches that should fix 8B field support (at least it works
> for me now):
> https://patches.dpdk.org/project/dpdk/list/?series=22676
> Please give it a try.

[idog] The patch works great for me. Thanx!

> In long term it probably would be good to hear from you and other users, should
> we keep 8B support at all, or might be it would be easier just to abandon it.
> Thanks
> Konstantin

[idog] I find the 8B option very useful:
1. It's easier and more natural to use for long size fields
e.g. part of how it simplifies our MAC rules code

@@ -231,48 +231,34 @@ struct rte_acl_field_def acl_fields[] = {
                {
                                .type = RTE_ACL_FIELD_TYPE_BITMASK,
-                               .size = sizeof(uint32_t),
-                               .field_index = FIELD_MAC_SRC_4MSB,
+                               .size = sizeof(uint64_t),
+                               .field_index = FIELD_MAC_SRC,
                                .input_index = INPUT_INDEX_GROUP_2,
                                .offset = offsetof(struct acl_data, mac_src),
                },
-              {
-                                .type = RTE_ACL_FIELD_TYPE_BITMASK,
-                               .size = sizeof(uint16_t),
-                               .field_index = FIELD_MAC_SRC_2LSB,
-                               .input_index = INPUT_INDEX_GROUP_3,
-                               .offset = offsetof(struct acl_data, mac_src) + sizeof(uint32_t),
-               },
.
.
.
+static int get_mac_val(const char *in, uint64_t *mac)
 {
-       static const size_t MAC_4MSB_SIZE = sizeof(uint32_t);
-       static const size_t MAC_2LSB_SIZE = sizeof(uint16_t);
        uint32_t i = 0;
        uint8_t octet = 0;
        char dlm = ':';
-
-       for (i = 0; i < MAC_4MSB_SIZE; i++)
-       {
-               GET_CB_FIELD(in, octet, 16, UINT8_MAX, dlm);
-               ((uint8_t*)mac4msb)[MAC_4MSB_SIZE - 1 - i] = octet;
-       }
-       for (i = 0; i < MAC_2LSB_SIZE; i++)
+       *mac = 0;
+       for (i = 0; i < RTE_ETHER_ADDR_LEN; i++)
        {
-               if (i ==  MAC_2LSB_SIZE - 1)
+               if (i == RTE_ETHER_ADDR_LEN - 1)
                        dlm = 0;
                GET_CB_FIELD(in, octet, 16, UINT8_MAX, dlm);
-               ((uint8_t*)mac2lsb)[MAC_2LSB_SIZE - 1 - i] = octet;
+               ((uint8_t*)mac)[RTE_ETHER_ADDR_LEN + 1 - i] = octet;
        }
        return 0;
 }

It' even much more significant for RTE_ACL_FIELD_TYPE_RANGE that may require
breaking a single U64 range to 3 U32 based rules

2. It may save acl fields
Alternative is to increase RTE_ACL_MAX_FIELDS (maybe expose it to rte_config.h)
As long as the "64" doesn't stand for some algorithmic/performance reason

Re: Does ACL support field size of 8 bytes?

[Konstantin Ananyev]

11/05/2022 15:28, Ido Goshen пишет:
> 
> 
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Tuesday, 26 April 2022 20:57
>> To: Ido Goshen <Ido@cgstowernetworks.com>; users@dpdk.org;
>> dev@dpdk.org
>> Cc: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>> Subject: RE: Does ACL support field size of 8 bytes?
>>
>>
>> Hi Ido,
>>
>>> I've lots of good experience with ACL but can't make it work with u64
>>> values I know it can be split to 2xu32 fields, but it makes it more
>>> complex to use and a wastes double  number of fields (we hit the
>>> RTE_ACL_MAX_FIELDS 64 limit)
>>
>> Wow, that's a lot of fields...
> 
> [idog]
> We provide a general purpose packet-broker that covers wide range of
> l2-l4 protocols + tunnels + some app level metadata.
> Though in most cases they won't be used simultaneously and many fields
> may end up being don't-care (e.g. mask=0) it's easier to code a static
> rte_acl_field_def struct that covers all the options then constructing it
> dynamically on each user configuration change
>   
>>> According to the documentation and rte_acl.h fields size can be 8[idog]
>>> ...
>>> Should it work?
>>> Did anyone try it successfully and/or can share an example?
>>
>> You are right: though it is formally supported, we do not test it, and AFAIK no-
>> one used it till now.
>> As we do group fields by 4B long chunks anyway, 8B field is sort of awkward and
>> confusing.
>> To be honest, I don't even remember what was the rationale beyond introducing
>> it at first place.
>> Anyway, just submitted patches that should fix 8B field support (at least it works
>> for me now):
>> https://patches.dpdk.org/project/dpdk/list/?series=22676
>> Please give it a try.
> 
> [idog] The patch works great for me. Thanx!
> 
>> In long term it probably would be good to hear from you and other users, should
>> we keep 8B support at all, or might be it would be easier just to abandon it.
>> Thanks
>> Konstantin
> 
> [idog] I find the 8B option very useful:
> 1. It's easier and more natural to use for long size fields
> e.g. part of how it simplifies our MAC rules code
> 
> @@ -231,48 +231,34 @@ struct rte_acl_field_def acl_fields[] = {
>                  {
>                                  .type = RTE_ACL_FIELD_TYPE_BITMASK,
> -                               .size = sizeof(uint32_t),
> -                               .field_index = FIELD_MAC_SRC_4MSB,
> +                               .size = sizeof(uint64_t),
> +                               .field_index = FIELD_MAC_SRC,
>                                  .input_index = INPUT_INDEX_GROUP_2,
>                                  .offset = offsetof(struct acl_data, mac_src),
>                  },
> -              {
> -                                .type = RTE_ACL_FIELD_TYPE_BITMASK,
> -                               .size = sizeof(uint16_t),
> -                               .field_index = FIELD_MAC_SRC_2LSB,
> -                               .input_index = INPUT_INDEX_GROUP_3,
> -                               .offset = offsetof(struct acl_data, mac_src) + sizeof(uint32_t),
> -               },
> .
> .
> .
> +static int get_mac_val(const char *in, uint64_t *mac)
>   {
> -       static const size_t MAC_4MSB_SIZE = sizeof(uint32_t);
> -       static const size_t MAC_2LSB_SIZE = sizeof(uint16_t);
>          uint32_t i = 0;
>          uint8_t octet = 0;
>          char dlm = ':';
> -
> -       for (i = 0; i < MAC_4MSB_SIZE; i++)
> -       {
> -               GET_CB_FIELD(in, octet, 16, UINT8_MAX, dlm);
> -               ((uint8_t*)mac4msb)[MAC_4MSB_SIZE - 1 - i] = octet;
> -       }
> -       for (i = 0; i < MAC_2LSB_SIZE; i++)
> +       *mac = 0;
> +       for (i = 0; i < RTE_ETHER_ADDR_LEN; i++)
>          {
> -               if (i ==  MAC_2LSB_SIZE - 1)
> +               if (i == RTE_ETHER_ADDR_LEN - 1)
>                          dlm = 0;
>                  GET_CB_FIELD(in, octet, 16, UINT8_MAX, dlm);
> -               ((uint8_t*)mac2lsb)[MAC_2LSB_SIZE - 1 - i] = octet;
> +               ((uint8_t*)mac)[RTE_ETHER_ADDR_LEN + 1 - i] = octet;
>          }
>          return 0;
>   }
> 
> It' even much more significant for RTE_ACL_FIELD_TYPE_RANGE that may require
> breaking a single U64 range to 3 U32 based rules

My concern was it is sort of awkward in terms of input_field
value for rules with 8B long.
But sure, if you believe it is useful, then let's try to keep it.
I submitted v2, there is no change in the library itself,
just updated the test script to cover new case.
If you'll have a chance, please add 'tested-by:' tag to it.

> 
> 2. It may save acl fields
> Alternative is to increase RTE_ACL_MAX_FIELDS (maybe expose it to rte_config.h)
> As long as the "64" doesn't stand for some algorithmic/performance reason

I kept RTE_ACL_MAX_FIELDS, but internally had to increase max number of 
input fields up to 2 * RTE_ACL_MAX_FIELDS, to cover the situation
when all fields are 8B long.

Thanks
Konstantin

RE: Does ACL support field size of 8 bytes?

[Ido Goshen]

> -----Original Message-----
> From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
> Sent: Sunday, 15 May 2022 23:54
> To: Ido Goshen <Ido@cgstowernetworks.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; users@dpdk.org; dev@dpdk.org
> Subject: Re: Does ACL support field size of 8 bytes?
> 
> 
> My concern was it is sort of awkward in terms of input_field value for rules with
> 8B long.

[idog] I'm always puzzled with the input_index field. 
I just randomly group the small size fields (u8, u16) without any applicative meaning.
Feels like it's redundant in the API an can be done internally by the lib like you do now for u64  
(though it'll be less trivial to do) 

Re: Does ACL support field size of 8 bytes?

[Konstantin Ananyev]

>> -----Original Message-----
>> From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>> Sent: Sunday, 15 May 2022 23:54
>> To: Ido Goshen <Ido@cgstowernetworks.com>; Ananyev, Konstantin
>> <konstantin.ananyev@intel.com>; users@dpdk.org; dev@dpdk.org
>> Subject: Re: Does ACL support field size of 8 bytes?
>>
>>
>> My concern was it is sort of awkward in terms of input_field value for rules with
>> 8B long.
> 
> [idog] I'm always puzzled with the input_index field.
> I just randomly group the small size fields (u8, u16) without any applicative meaning.
> Feels like it's redundant in the API an can be done internally by the lib like you do now for u64
> (though it'll be less trivial to do)
> 

Yep agree, field_index and offset - seems enough,
other stuff can probably be figured out by analyzing these two fields.
Though as you said, it would require some extra effort.