[dpdk-dev,01/18] net/ixgbe: store SYN filter
Checks
Commit Message
From: wei zhao1 <wei.zhao1@intel.com>
Add support for storing SYN filter in SW.
Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
---
drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
2 files changed, 12 insertions(+), 2 deletions(-)
Comments
On 12/2/2016 10:42 AM, Wei Zhao wrote:
> From: wei zhao1 <wei.zhao1@intel.com>
>
> Add support for storing SYN filter in SW.
Do you think does it makes more clear to refer as TCP SYN filter? Or SYN
filter is clear enough?
>
> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
Can you please update sign-off to your actual name?
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
> drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index edc9b22..7f10cca 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
> memset(filter_info->fivetuple_mask, 0,
> sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
>
> + /* initialize SYN filter */
> + filter_info->syn_info = 0;
can it be an option to memset all filter_info? (and of course move list
init after memset)
> return 0;
> }
>
> @@ -5509,15 +5511,19 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> bool add)
> {
> struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct ixgbe_filter_info *filter_info =
> + IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data->dev_private);
> + uint32_t syn_info;
> uint32_t synqf;
>
> if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> return -EINVAL;
>
> + syn_info = filter_info->syn_info;
> synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF);
>
> if (add) {
> - if (synqf & IXGBE_SYN_FILTER_ENABLE)
> + if (syn_info & IXGBE_SYN_FILTER_ENABLE)
If these checks will be done on syn_info, shouldn't syn_info be assigned
to synqf before this. Specially for first usage, synqf may be different
than hw register.
Or perhaps can keep continue to use synqf. Since synqf assigned to
filter_info->syn_info after updated.
> return -EINVAL;
> synqf = (uint32_t)(((filter->queue << IXGBE_SYN_FILTER_QUEUE_SHIFT) &
> IXGBE_SYN_FILTER_QUEUE) | IXGBE_SYN_FILTER_ENABLE);
> @@ -5527,10 +5533,12 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> else
> synqf &= ~IXGBE_SYN_FILTER_SYNQFP;
> } else {
> - if (!(synqf & IXGBE_SYN_FILTER_ENABLE))
> + if (!(syn_info & IXGBE_SYN_FILTER_ENABLE))
> return -ENOENT;
> synqf &= ~(IXGBE_SYN_FILTER_QUEUE | IXGBE_SYN_FILTER_ENABLE);
> }
> +
> + filter_info->syn_info = synqf;
> IXGBE_WRITE_REG(hw, IXGBE_SYNQF, synqf);
> IXGBE_WRITE_FLUSH(hw);
> return 0;
<...>
Hi, Yigit
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:56 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
>
> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for storing SYN filter in SW.
>
> Do you think does it makes more clear to refer as TCP SYN filter? Or SYN filter
> is clear enough?
>
Ok, I will change to " TCP SYN filter " to make it more clear
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
>
> Can you please update sign-off to your actual name?
>
Ok, I will change to " Signed-off-by: Wei Zhao <wei.zhao1@intel.com>"
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
> > drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> > 2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index edc9b22..7f10cca 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
> > memset(filter_info->fivetuple_mask, 0,
> > sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
> >
> > + /* initialize SYN filter */
> > + filter_info->syn_info = 0;
>
> can it be an option to memset all filter_info? (and of course move list init
> after memset)
>
Maybe, change to the following code?
memset(filter_info, 0, sizeof(struct ixgbe_filter_info));
TAILQ_INIT(&filter_info->fivetuple_list);
But that wiil mix /* initialize ether type filter */ and /* initialize 5tuple filter list */ two process together,
Because struct ixgbe_filter_info store two type info of ether and 5tuple.
So, not to change ?
struct ixgbe_filter_info {
uint8_t ethertype_mask; /* Bit mask for every used ethertype filter */
/* store used ethertype filters*/
struct ixgbe_ethertype_filter ethertype_filters[IXGBE_MAX_ETQF_FILTERS];
/* Bit mask for every used 5tuple filter */
uint32_t fivetuple_mask[IXGBE_5TUPLE_ARRAY_SIZE];
struct ixgbe_5tuple_filter_list fivetuple_list;
/* store the SYN filter info */
uint32_t syn_info;
};
> > return 0;
> > }
> >
> > @@ -5509,15 +5511,19 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> > bool add)
> > {
> > struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > + struct ixgbe_filter_info *filter_info =
> > + IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data-
> >dev_private);
> > + uint32_t syn_info;
> > uint32_t synqf;
> >
> > if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> > return -EINVAL;
> >
> > + syn_info = filter_info->syn_info;
> > synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF);
> >
> > if (add) {
> > - if (synqf & IXGBE_SYN_FILTER_ENABLE)
> > + if (syn_info & IXGBE_SYN_FILTER_ENABLE)
>
> If these checks will be done on syn_info, shouldn't syn_info be assigned to
> synqf before this. Specially for first usage, synqf may be different than hw
> register.
>
> Or perhaps can keep continue to use synqf. Since synqf assigned to
> filter_info->syn_info after updated.
>
Let me have a deeper think of this to reply you.
> > return -EINVAL;
> > synqf = (uint32_t)(((filter->queue <<
> IXGBE_SYN_FILTER_QUEUE_SHIFT) &
> > IXGBE_SYN_FILTER_QUEUE) |
> IXGBE_SYN_FILTER_ENABLE); @@ -5527,10
> > +5533,12 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> > else
> > synqf &= ~IXGBE_SYN_FILTER_SYNQFP;
> > } else {
> > - if (!(synqf & IXGBE_SYN_FILTER_ENABLE))
> > + if (!(syn_info & IXGBE_SYN_FILTER_ENABLE))
> > return -ENOENT;
> > synqf &= ~(IXGBE_SYN_FILTER_QUEUE |
> IXGBE_SYN_FILTER_ENABLE);
> > }
> > +
> > + filter_info->syn_info = synqf;
> > IXGBE_WRITE_REG(hw, IXGBE_SYNQF, synqf);
> > IXGBE_WRITE_FLUSH(hw);
> > return 0;
> <...>
>
Hi, Ferruh
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, December 21, 2016 12:56 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
>
> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> > From: wei zhao1 <wei.zhao1@intel.com>
> >
> > Add support for storing SYN filter in SW.
>
> Do you think does it makes more clear to refer as TCP SYN filter? Or SYN filter
> is clear enough?
>
> >
> > Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> > Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
>
> Can you please update sign-off to your actual name?
>
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
> > drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> > 2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index edc9b22..7f10cca 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
> > memset(filter_info->fivetuple_mask, 0,
> > sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
> >
> > + /* initialize SYN filter */
> > + filter_info->syn_info = 0;
>
> can it be an option to memset all filter_info? (and of course move list init
> after memset)
>
> > return 0;
> > }
> >
> > @@ -5509,15 +5511,19 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> > bool add)
> > {
> > struct ixgbe_hw *hw =
> > IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > + struct ixgbe_filter_info *filter_info =
> > + IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data-
> >dev_private);
> > + uint32_t syn_info;
> > uint32_t synqf;
> >
> > if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
> > return -EINVAL;
> >
> > + syn_info = filter_info->syn_info;
> > synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF);
> >
> > if (add) {
> > - if (synqf & IXGBE_SYN_FILTER_ENABLE)
> > + if (syn_info & IXGBE_SYN_FILTER_ENABLE)
>
> If these checks will be done on syn_info, shouldn't syn_info be assigned to
> synqf before this. Specially for first usage, synqf may be different than hw
> register.
>
> Or perhaps can keep continue to use synqf. Since synqf assigned to
> filter_info->syn_info after updated.
>
ok, this code is alittle vague, in "add" branch synqf will be assigned a new value, so "synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF)" is useless.
synqf read from hw only to be used in "else" branch.so I will make a little code change here.
Thank you for your suggestion.
> > return -EINVAL;
> > synqf = (uint32_t)(((filter->queue <<
> IXGBE_SYN_FILTER_QUEUE_SHIFT) &
> > IXGBE_SYN_FILTER_QUEUE) |
> IXGBE_SYN_FILTER_ENABLE); @@ -5527,10
> > +5533,12 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
> > else
> > synqf &= ~IXGBE_SYN_FILTER_SYNQFP;
> > } else {
> > - if (!(synqf & IXGBE_SYN_FILTER_ENABLE))
> > + if (!(syn_info & IXGBE_SYN_FILTER_ENABLE))
> > return -ENOENT;
> > synqf &= ~(IXGBE_SYN_FILTER_QUEUE |
> IXGBE_SYN_FILTER_ENABLE);
> > }
> > +
> > + filter_info->syn_info = synqf;
> > IXGBE_WRITE_REG(hw, IXGBE_SYNQF, synqf);
> > IXGBE_WRITE_FLUSH(hw);
> > return 0;
> <...>
>
On 12/22/2016 9:48 AM, Zhao1, Wei wrote:
> Hi, Yigit
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, December 21, 2016 12:56 AM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
>>
>> On 12/2/2016 10:42 AM, Wei Zhao wrote:
>>> From: wei zhao1 <wei.zhao1@intel.com>
>>>
>>> Add support for storing SYN filter in SW.
>>
>> Do you think does it makes more clear to refer as TCP SYN filter? Or SYN filter
>> is clear enough?
>>
>
> Ok, I will change to " TCP SYN filter " to make it more clear
>
>>>
>>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>>> Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
>>
>> Can you please update sign-off to your actual name?
>>
>
> Ok, I will change to " Signed-off-by: Wei Zhao <wei.zhao1@intel.com>"
>
>>> ---
>>> drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
>>> drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
>>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> index edc9b22..7f10cca 100644
>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>> @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
>>> memset(filter_info->fivetuple_mask, 0,
>>> sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
>>>
>>> + /* initialize SYN filter */
>>> + filter_info->syn_info = 0;
>>
>> can it be an option to memset all filter_info? (and of course move list init
>> after memset)
>>
>
> Maybe, change to the following code?
>
> memset(filter_info, 0, sizeof(struct ixgbe_filter_info));
> TAILQ_INIT(&filter_info->fivetuple_list);
>
> But that wiil mix /* initialize ether type filter */ and /* initialize 5tuple filter list */ two process together,
> Because struct ixgbe_filter_info store two type info of ether and 5tuple.
I see filter info consist of different filter types, but as far as I can
see they are not used before this memset, so what is the problem of
cleaning all struct?
Currently memset a sub-set of struct, and assigns zero to other field
explicitly, and rest remains undefined and unused. I am suggesting
memset whole structure and get rid of zero assignment.
> So, not to change ?
>
> struct ixgbe_filter_info {
> uint8_t ethertype_mask; /* Bit mask for every used ethertype filter */
> /* store used ethertype filters*/
> struct ixgbe_ethertype_filter ethertype_filters[IXGBE_MAX_ETQF_FILTERS];
> /* Bit mask for every used 5tuple filter */
> uint32_t fivetuple_mask[IXGBE_5TUPLE_ARRAY_SIZE];
> struct ixgbe_5tuple_filter_list fivetuple_list;
> /* store the SYN filter info */
> uint32_t syn_info;
> };
>
>
<...>
Hi, yigit
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Saturday, January 7, 2017 12:26 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
>
> On 12/22/2016 9:48 AM, Zhao1, Wei wrote:
> > Hi, Yigit
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, December 21, 2016 12:56 AM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH 01/18] net/ixgbe: store SYN filter
> >>
> >> On 12/2/2016 10:42 AM, Wei Zhao wrote:
> >>> From: wei zhao1 <wei.zhao1@intel.com>
> >>>
> >>> Add support for storing SYN filter in SW.
> >>
> >> Do you think does it makes more clear to refer as TCP SYN filter? Or
> >> SYN filter is clear enough?
> >>
> >
> > Ok, I will change to " TCP SYN filter " to make it more clear
> >
> >>>
> >>> Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> >>> Signed-off-by: wei zhao1 <wei.zhao1@intel.com>
> >>
> >> Can you please update sign-off to your actual name?
> >>
> >
> > Ok, I will change to " Signed-off-by: Wei Zhao <wei.zhao1@intel.com>"
> >
> >>> ---
> >>> drivers/net/ixgbe/ixgbe_ethdev.c | 12 ++++++++++--
> >>> drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> >>> 2 files changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> index edc9b22..7f10cca 100644
> >>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >>> @@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev
> *eth_dev)
> >>> memset(filter_info->fivetuple_mask, 0,
> >>> sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
> >>>
> >>> + /* initialize SYN filter */
> >>> + filter_info->syn_info = 0;
> >>
> >> can it be an option to memset all filter_info? (and of course move
> >> list init after memset)
> >>
> >
> > Maybe, change to the following code?
> >
> > memset(filter_info, 0, sizeof(struct ixgbe_filter_info));
> > TAILQ_INIT(&filter_info->fivetuple_list);
> >
> > But that wiil mix /* initialize ether type filter */ and /* initialize
> > 5tuple filter list */ two process together, Because struct ixgbe_filter_info
> store two type info of ether and 5tuple.
>
> I see filter info consist of different filter types, but as far as I can see they are
> not used before this memset, so what is the problem of cleaning all struct?
>
> Currently memset a sub-set of struct, and assigns zero to other field explicitly,
> and rest remains undefined and unused. I am suggesting memset whole
> structure and get rid of zero assignment.
>
Ok, do as your suggestion in v3.
> > So, not to change ?
> >
> > struct ixgbe_filter_info {
> > uint8_t ethertype_mask; /* Bit mask for every used ethertype filter
> */
> > /* store used ethertype filters*/
> > struct ixgbe_ethertype_filter
> ethertype_filters[IXGBE_MAX_ETQF_FILTERS];
> > /* Bit mask for every used 5tuple filter */
> > uint32_t fivetuple_mask[IXGBE_5TUPLE_ARRAY_SIZE];
> > struct ixgbe_5tuple_filter_list fivetuple_list;
> > /* store the SYN filter info */
> > uint32_t syn_info;
> > };
> >
> >
> <...>
@@ -1287,6 +1287,8 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
memset(filter_info->fivetuple_mask, 0,
sizeof(uint32_t) * IXGBE_5TUPLE_ARRAY_SIZE);
+ /* initialize SYN filter */
+ filter_info->syn_info = 0;
return 0;
}
@@ -5509,15 +5511,19 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
bool add)
{
struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct ixgbe_filter_info *filter_info =
+ IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data->dev_private);
+ uint32_t syn_info;
uint32_t synqf;
if (filter->queue >= IXGBE_MAX_RX_QUEUE_NUM)
return -EINVAL;
+ syn_info = filter_info->syn_info;
synqf = IXGBE_READ_REG(hw, IXGBE_SYNQF);
if (add) {
- if (synqf & IXGBE_SYN_FILTER_ENABLE)
+ if (syn_info & IXGBE_SYN_FILTER_ENABLE)
return -EINVAL;
synqf = (uint32_t)(((filter->queue << IXGBE_SYN_FILTER_QUEUE_SHIFT) &
IXGBE_SYN_FILTER_QUEUE) | IXGBE_SYN_FILTER_ENABLE);
@@ -5527,10 +5533,12 @@ ixgbe_syn_filter_set(struct rte_eth_dev *dev,
else
synqf &= ~IXGBE_SYN_FILTER_SYNQFP;
} else {
- if (!(synqf & IXGBE_SYN_FILTER_ENABLE))
+ if (!(syn_info & IXGBE_SYN_FILTER_ENABLE))
return -ENOENT;
synqf &= ~(IXGBE_SYN_FILTER_QUEUE | IXGBE_SYN_FILTER_ENABLE);
}
+
+ filter_info->syn_info = synqf;
IXGBE_WRITE_REG(hw, IXGBE_SYNQF, synqf);
IXGBE_WRITE_FLUSH(hw);
return 0;
@@ -262,6 +262,8 @@ struct ixgbe_filter_info {
/* Bit mask for every used 5tuple filter */
uint32_t fivetuple_mask[IXGBE_5TUPLE_ARRAY_SIZE];
struct ixgbe_5tuple_filter_list fivetuple_list;
+ /* store the SYN filter info */
+ uint32_t syn_info;
};
/*