11/05/2022 15:28, Ido Goshen пишет:
-----Original Message-----
From: Ananyev, Konstantin <konstantin.anan...@intel.com>
Sent: Tuesday, 26 April 2022 20:57
To: Ido Goshen <i...@cgstowernetworks.com>; us...@dpdk.org;
dev@dpdk.org
Cc: Konstantin Ananyev <konstantin.v.anan...@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
