Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Hi, Ferruh
> -Original Message-
> From: Yigit, Ferruh
> Sent: Friday, September 22, 2017 5:01 AM
> To: Zhao1, Wei ; dev@dpdk.org
> Cc: Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
> On 9/21/2017 7:16 PM, Ferruh Yigit wrote:
> > On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> >> Hi，Ferruh
> >>
> >>> -Original Message-
> >>> From: Yigit, Ferruh
> >>> Sent: Thursday, September 14, 2017 9:31 PM
> >>> To: Zhao1, Wei ; dev@dpdk.org
> >>> Cc: Wu, Jingjing
> >>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats
> >>> bug in vf port
> >>>
> >>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> >>>> Hi, Ferruh
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Yigit, Ferruh
> >>>>> Sent: Friday, September 1, 2017 12:54 AM
> >>>>> To: Zhao1, Wei ; dev@dpdk.org
> >>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats
> >>>>> bug in vf port
> >>>>>
> >>>>> On 8/29/2017 3:28 AM, Wei Zhao wrote:
> >>>>>> There is a bug in vf clear xstats command, it do not record the
> >>>>>> statics data in offset struct member.So, vf need to keep record
> >>>>>> of xstats data from pf and update the statics according to offset.
> >>>>>>
> >>>>>> Fixes: da61cd0849766 ("i40evf: add extended stats")
> >>>>>>
> >>>>>> Signed-off-by: Wei Zhao
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>>
> >>>>>> fix patch log check warning.
> >>>>>> ---
> >>>>>> app/test-pmd/config.c | 6 ++--
> >>>>>> drivers/net/i40e/i40e_ethdev_vf.c | 64
> >>>>>> ++-
> >>>>>> 2 files changed, 67 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>>>> 3ae3e1c..14131d6 100644
> >>>>>> --- a/app/test-pmd/config.c
> >>>>>> +++ b/app/test-pmd/config.c
> >>>>>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> >>>>>>if (diff_cycles > 0)
> >>>>>>diff_cycles = prev_cycles[port_id] - diff_cycles;
> >>>>>>
> >>>>>> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> >>>>>> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> >>>>>> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> >>>>>> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> >>>>>> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> >>>>>> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
> >>>>>
> >>>>> I guess this testpmd update is not directly related to this patch,
> >>>>> but to protect testpmd against value overflow? Can this be another
> patch?
> >>>>
> >>>> Nonono, this code change is directly related to this patch, if we
> >>>> do not do this code change, the diff_pkts_rx and diff_pkts_tx
> >>>> statistic data will
> >>> be wrong when the first time after clear xstats command.
> >>>
> >>> If this testpmd code is only wrong for i40e vf after this patch,
> >>> perhaps something else is wrong? Perhaps we should update i40e vf
> stats.
> >>>
> >>> OR, if this code is already wrong, lets move it to its own patch.
> >>>
> >>
> >> A new patch will be commit later.
> >>
> >>>>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>> static int
> >>>>>> i40evf_get_statistics(struct rte_eth_dev *dev, struct
> >>>>>> rte_eth_stats
> >>>>>> *stats) {
> >>>>>>int ret;
> >>>>>>struct i40e_eth_stats *pstats = NULL;
> >>>>>> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev-
> >data-
> &g
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
> -Original Message-
> From: Yigit, Ferruh
> Sent: Friday, September 22, 2017 2:16 AM
> To: Zhao1, Wei ; dev@dpdk.org
> Cc: Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
> On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> > Hi，Ferruh
> >
> >> -Original Message-
> >> From: Yigit, Ferruh
> >> Sent: Thursday, September 14, 2017 9:31 PM
> >> To: Zhao1, Wei ; dev@dpdk.org
> >> Cc: Wu, Jingjing
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> >> in vf port
> >>
> >> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> >>> Hi, Ferruh
> >>>
> >>>> -Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Friday, September 1, 2017 12:54 AM
> >>>> To: Zhao1, Wei ; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats
> >>>> bug in vf port
> >>>>
> >>>> On 8/29/2017 3:28 AM, Wei Zhao wrote:
> >>>>> There is a bug in vf clear xstats command, it do not record the
> >>>>> statics data in offset struct member.So, vf need to keep record of
> >>>>> xstats data from pf and update the statics according to offset.
> >>>>>
> >>>>> Fixes: da61cd0849766 ("i40evf: add extended stats")
> >>>>>
> >>>>> Signed-off-by: Wei Zhao
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Changes in v2:
> >>>>>
> >>>>> fix patch log check warning.
> >>>>> ---
> >>>>> app/test-pmd/config.c | 6 ++--
> >>>>> drivers/net/i40e/i40e_ethdev_vf.c | 64
> >>>>> ++-
> >>>>> 2 files changed, 67 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>>> 3ae3e1c..14131d6 100644
> >>>>> --- a/app/test-pmd/config.c
> >>>>> +++ b/app/test-pmd/config.c
> >>>>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> >>>>> if (diff_cycles > 0)
> >>>>> diff_cycles = prev_cycles[port_id] - diff_cycles;
> >>>>>
> >>>>> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> >>>>> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> >>>>> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> >>>>> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> >>>>> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> >>>>> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
> >>>>
> >>>> I guess this testpmd update is not directly related to this patch,
> >>>> but to protect testpmd against value overflow? Can this be another
> patch?
> >>>
> >>> Nonono, this code change is directly related to this patch, if we do
> >>> not do this code change, the diff_pkts_rx and diff_pkts_tx statistic
> >>> data will
> >> be wrong when the first time after clear xstats command.
> >>
> >> If this testpmd code is only wrong for i40e vf after this patch,
> >> perhaps something else is wrong? Perhaps we should update i40e vf stats.
> >>
> >> OR, if this code is already wrong, lets move it to its own patch.
> >>
> >
> > A new patch will be commit later.
> >
> >>>
> >>>>
> >>>> <...>
> >>>>
> >>>>> static int
> >>>>> i40evf_get_statistics(struct rte_eth_dev *dev, struct
> >>>>> rte_eth_stats
> >>>>> *stats) {
> >>>>> int ret;
> >>>>> struct i40e_eth_stats *pstats = NULL;
> >>>>> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev-
> >data-
> >>>>> dev_private);
> >>>>> + struct i40e_vsi *vsi = &vf->vsi;
> >>>>>
> >>>>> ret = i40evf_update_stats(dev, &pstats);
> >>>>> if (ret != 0)
> >>>>> return 0;
> >>>>>
> >>>>> + i40evf_update_
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
On 9/21/2017 7:16 PM, Ferruh Yigit wrote:
> On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
>> Hi，Ferruh
>>
>>> -Original Message-
>>> From: Yigit, Ferruh
>>> Sent: Thursday, September 14, 2017 9:31 PM
>>> To: Zhao1, Wei ; dev@dpdk.org
>>> Cc: Wu, Jingjing
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
>>> port
>>>
>>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
>>>> Hi, Ferruh
>>>>
>>>>> -Original Message-
>>>>> From: Yigit, Ferruh
>>>>> Sent: Friday, September 1, 2017 12:54 AM
>>>>> To: Zhao1, Wei ; dev@dpdk.org
>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
>>>>> in vf port
>>>>>
>>>>> On 8/29/2017 3:28 AM, Wei Zhao wrote:
>>>>>> There is a bug in vf clear xstats command, it do not record the
>>>>>> statics data in offset struct member.So, vf need to keep record of
>>>>>> xstats data from pf and update the statics according to offset.
>>>>>>
>>>>>> Fixes: da61cd0849766 ("i40evf: add extended stats")
>>>>>>
>>>>>> Signed-off-by: Wei Zhao
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>>
>>>>>> fix patch log check warning.
>>>>>> ---
>>>>>> app/test-pmd/config.c | 6 ++--
>>>>>> drivers/net/i40e/i40e_ethdev_vf.c | 64
>>>>>> ++-
>>>>>> 2 files changed, 67 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>> 3ae3e1c..14131d6 100644
>>>>>> --- a/app/test-pmd/config.c
>>>>>> +++ b/app/test-pmd/config.c
>>>>>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
>>>>>> if (diff_cycles > 0)
>>>>>> diff_cycles = prev_cycles[port_id] - diff_cycles;
>>>>>>
>>>>>> -diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
>>>>>> -diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
>>>>>> +diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
>>>>>> +(stats.ipackets - prev_pkts_rx[port_id]) : 0;
>>>>>> +diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
>>>>>> +(stats.opackets - prev_pkts_tx[port_id]) : 0;
>>>>>
>>>>> I guess this testpmd update is not directly related to this patch,
>>>>> but to protect testpmd against value overflow? Can this be another patch?
>>>>
>>>> Nonono, this code change is directly related to this patch, if we do
>>>> not do this code change, the diff_pkts_rx and diff_pkts_tx statistic data
>>>> will
>>> be wrong when the first time after clear xstats command.
>>>
>>> If this testpmd code is only wrong for i40e vf after this patch, perhaps
>>> something else is wrong? Perhaps we should update i40e vf stats.
>>>
>>> OR, if this code is already wrong, lets move it to its own patch.
>>>
>>
>> A new patch will be commit later.
>>
>>>>
>>>>>
>>>>> <...>
>>>>>
>>>>>> static int
>>>>>> i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
>>>>>> *stats) {
>>>>>> int ret;
>>>>>> struct i40e_eth_stats *pstats = NULL;
>>>>>> +struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
>>>>>> dev_private);
>>>>>> +struct i40e_vsi *vsi = &vf->vsi;
>>>>>>
>>>>>> ret = i40evf_update_stats(dev, &pstats);
>>>>>> if (ret != 0)
>>>>>> return 0;
>>>>>>
>>>>>> +i40evf_update_vsi_stats(vsi, pstats);
>>>>>
>>>>> But not having this previously means all VF stats were wrong
>>>>> previously, not only extended ones, also basic ones. And not not
>>>>> wrong with small difference, this should give a big difference in the
>>>>> stats.
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
On 9/21/2017 4:11 AM, Zhao1, Wei wrote:
> Hi，Ferruh
>
>> -Original Message-
>> From: Yigit, Ferruh
>> Sent: Thursday, September 14, 2017 9:31 PM
>> To: Zhao1, Wei ; dev@dpdk.org
>> Cc: Wu, Jingjing
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
>> port
>>
>> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
>>> Hi, Ferruh
>>>
>>>> -Original Message-
>>>> From: Yigit, Ferruh
>>>> Sent: Friday, September 1, 2017 12:54 AM
>>>> To: Zhao1, Wei ; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
>>>> in vf port
>>>>
>>>> On 8/29/2017 3:28 AM, Wei Zhao wrote:
>>>>> There is a bug in vf clear xstats command, it do not record the
>>>>> statics data in offset struct member.So, vf need to keep record of
>>>>> xstats data from pf and update the statics according to offset.
>>>>>
>>>>> Fixes: da61cd0849766 ("i40evf: add extended stats")
>>>>>
>>>>> Signed-off-by: Wei Zhao
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>>
>>>>> fix patch log check warning.
>>>>> ---
>>>>> app/test-pmd/config.c | 6 ++--
>>>>> drivers/net/i40e/i40e_ethdev_vf.c | 64
>>>>> ++-
>>>>> 2 files changed, 67 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>> 3ae3e1c..14131d6 100644
>>>>> --- a/app/test-pmd/config.c
>>>>> +++ b/app/test-pmd/config.c
>>>>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
>>>>> if (diff_cycles > 0)
>>>>> diff_cycles = prev_cycles[port_id] - diff_cycles;
>>>>>
>>>>> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
>>>>> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
>>>>> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
>>>>> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
>>>>> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
>>>>> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
>>>>
>>>> I guess this testpmd update is not directly related to this patch,
>>>> but to protect testpmd against value overflow? Can this be another patch?
>>>
>>> Nonono, this code change is directly related to this patch, if we do
>>> not do this code change, the diff_pkts_rx and diff_pkts_tx statistic data
>>> will
>> be wrong when the first time after clear xstats command.
>>
>> If this testpmd code is only wrong for i40e vf after this patch, perhaps
>> something else is wrong? Perhaps we should update i40e vf stats.
>>
>> OR, if this code is already wrong, lets move it to its own patch.
>>
>
> A new patch will be commit later.
>
>>>
>>>>
>>>> <...>
>>>>
>>>>> static int
>>>>> i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
>>>>> *stats) {
>>>>> int ret;
>>>>> struct i40e_eth_stats *pstats = NULL;
>>>>> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
>>>>> dev_private);
>>>>> + struct i40e_vsi *vsi = &vf->vsi;
>>>>>
>>>>> ret = i40evf_update_stats(dev, &pstats);
>>>>> if (ret != 0)
>>>>> return 0;
>>>>>
>>>>> + i40evf_update_vsi_stats(vsi, pstats);
>>>>
>>>> But not having this previously means all VF stats were wrong
>>>> previously, not only extended ones, also basic ones. And not not
>>>> wrong with small difference, this should give a big difference in the
>>>> stats.
>>>>
>>>> I am suspicious about this part, because if this is the case, I would
>>>> expect this should be detected earlier.
>>>>
>>>> I have not traced the code, but is there any chance that
>> "eth_stats_offset"
>>>> has been used by other end of the admin command?
>>>
>>> To be frankly speaking, this bug is firstly discovered by a big user.
>>> This bug only appear after use CLI "clear port
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Hi，Ferruh
> -Original Message-
> From: Yigit, Ferruh
> Sent: Thursday, September 14, 2017 9:31 PM
> To: Zhao1, Wei ; dev@dpdk.org
> Cc: Wu, Jingjing
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
> On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> > Hi, Ferruh
> >
> >> -Original Message-
> >> From: Yigit, Ferruh
> >> Sent: Friday, September 1, 2017 12:54 AM
> >> To: Zhao1, Wei ; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> >> in vf port
> >>
> >> On 8/29/2017 3:28 AM, Wei Zhao wrote:
> >>> There is a bug in vf clear xstats command, it do not record the
> >>> statics data in offset struct member.So, vf need to keep record of
> >>> xstats data from pf and update the statics according to offset.
> >>>
> >>> Fixes: da61cd0849766 ("i40evf: add extended stats")
> >>>
> >>> Signed-off-by: Wei Zhao
> >>>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>
> >>> fix patch log check warning.
> >>> ---
> >>> app/test-pmd/config.c | 6 ++--
> >>> drivers/net/i40e/i40e_ethdev_vf.c | 64
> >>> ++-
> >>> 2 files changed, 67 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>> 3ae3e1c..14131d6 100644
> >>> --- a/app/test-pmd/config.c
> >>> +++ b/app/test-pmd/config.c
> >>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> >>> if (diff_cycles > 0)
> >>> diff_cycles = prev_cycles[port_id] - diff_cycles;
> >>>
> >>> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> >>> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> >>> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> >>> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> >>> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> >>> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
> >>
> >> I guess this testpmd update is not directly related to this patch,
> >> but to protect testpmd against value overflow? Can this be another patch?
> >
> > Nonono, this code change is directly related to this patch, if we do
> > not do this code change, the diff_pkts_rx and diff_pkts_tx statistic data
> > will
> be wrong when the first time after clear xstats command.
>
> If this testpmd code is only wrong for i40e vf after this patch, perhaps
> something else is wrong? Perhaps we should update i40e vf stats.
>
> OR, if this code is already wrong, lets move it to its own patch.
>
A new patch will be commit later.
> >
> >>
> >> <...>
> >>
> >>> static int
> >>> i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
> >>> *stats) {
> >>> int ret;
> >>> struct i40e_eth_stats *pstats = NULL;
> >>> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> >>> dev_private);
> >>> + struct i40e_vsi *vsi = &vf->vsi;
> >>>
> >>> ret = i40evf_update_stats(dev, &pstats);
> >>> if (ret != 0)
> >>> return 0;
> >>>
> >>> + i40evf_update_vsi_stats(vsi, pstats);
> >>
> >> But not having this previously means all VF stats were wrong
> >> previously, not only extended ones, also basic ones. And not not
> >> wrong with small difference, this should give a big difference in the
> >> stats.
> >>
> >> I am suspicious about this part, because if this is the case, I would
> >> expect this should be detected earlier.
> >>
> >> I have not traced the code, but is there any chance that
> "eth_stats_offset"
> >> has been used by other end of the admin command?
> >
> > To be frankly speaking, this bug is firstly discovered by a big user.
> > This bug only appear after use CLI "clear port xstats 0". So it is not easy
> > to
> detect this bug.
> > After using this fix patch ,the big user who report this issue has feed
> > back it
> work well now.
> > The root cause is not so complicated, when the pf which admin this vf
> > is in kernel state, DPDK can not Give pf the info to clear and update
> > offset command, so vf ca
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
On 9/1/2017 3:30 AM, Zhao1, Wei wrote:
> Hi, Ferruh
>
>> -Original Message-
>> From: Yigit, Ferruh
>> Sent: Friday, September 1, 2017 12:54 AM
>> To: Zhao1, Wei ; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
>> port
>>
>> On 8/29/2017 3:28 AM, Wei Zhao wrote:
>>> There is a bug in vf clear xstats command, it do not record the
>>> statics data in offset struct member.So, vf need to keep record of
>>> xstats data from pf and update the statics according to offset.
>>>
>>> Fixes: da61cd0849766 ("i40evf: add extended stats")
>>>
>>> Signed-off-by: Wei Zhao
>>>
>>> ---
>>>
>>> Changes in v2:
>>>
>>> fix patch log check warning.
>>> ---
>>> app/test-pmd/config.c | 6 ++--
>>> drivers/net/i40e/i40e_ethdev_vf.c | 64
>>> ++-
>>> 2 files changed, 67 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>> 3ae3e1c..14131d6 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
>>> if (diff_cycles > 0)
>>> diff_cycles = prev_cycles[port_id] - diff_cycles;
>>>
>>> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
>>> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
>>> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
>>> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
>>> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
>>> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
>>
>> I guess this testpmd update is not directly related to this patch, but to
>> protect
>> testpmd against value overflow? Can this be another patch?
>
> Nonono, this code change is directly related to this patch, if we do not do
> this code change, the
> diff_pkts_rx and diff_pkts_tx statistic data will be wrong when the first
> time after clear xstats command.
If this testpmd code is only wrong for i40e vf after this patch, perhaps
something else is wrong? Perhaps we should update i40e vf stats.
OR, if this code is already wrong, lets move it to its own patch.
>
>>
>> <...>
>>
>>> static int
>>> i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
>>> *stats) {
>>> int ret;
>>> struct i40e_eth_stats *pstats = NULL;
>>> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
>>> dev_private);
>>> + struct i40e_vsi *vsi = &vf->vsi;
>>>
>>> ret = i40evf_update_stats(dev, &pstats);
>>> if (ret != 0)
>>> return 0;
>>>
>>> + i40evf_update_vsi_stats(vsi, pstats);
>>
>> But not having this previously means all VF stats were wrong previously, not
>> only extended ones, also basic ones. And not not wrong with small
>> difference, this should give a big difference in the stats.
>>
>> I am suspicious about this part, because if this is the case, I would expect
>> this
>> should be detected earlier.
>>
>> I have not traced the code, but is there any chance that "eth_stats_offset"
>> has been used by other end of the admin command?
>
> To be frankly speaking, this bug is firstly discovered by a big user.
> This bug only appear after use CLI "clear port xstats 0". So it is not easy
> to detect this bug.
> After using this fix patch ,the big user who report this issue has feed back
> it work well now.
> The root cause is not so complicated, when the pf which admin this vf is in
> kernel state, DPDK can not
> Give pf the info to clear and update offset command, so vf can only keep
> record the offset data in DPDK
> VF port locally.
Please help me understand this.
1- The problem you are fixing only seen with Linux PF, with DPDK PF you
don't see the problem, correct? If so this should be part of commit log.
2- As I checked the Linux driver code, it does same thing with DPDK:
a) in PF side, read from registers
b) removed vsi->eth_stats_offsets from read values
c) set vsi->eth_stats
So vsi->eth_stats should be valid, can you please elaborate the issue
with Linux PF.
3- This patch introduces i40evf_update_vsi_stats(), which removes
vsi->eth_stats_offset from stats received from PF.
But for DPDK PF case, the stats received from PF are already removes
vsi->eth_stats_offset, won&#x
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Hi, jingjing
> -Original Message-
> From: Wu, Jingjing
> Sent: Saturday, September 9, 2017 11:16 AM
> To: Zhao1, Wei ; Yigit, Ferruh
> ; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
>
>
> > -Original Message-
> > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Zhao1, Wei
> > Sent: Friday, September 1, 2017 10:30 AM
> > To: Yigit, Ferruh ; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug
> > in vf port
> >
> > Hi, Ferruh
> >
> > > -Original Message-
> > > From: Yigit, Ferruh
> > > Sent: Friday, September 1, 2017 12:54 AM
> > > To: Zhao1, Wei ; dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats
> > > bug in vf port
> > >
> > > On 8/29/2017 3:28 AM, Wei Zhao wrote:
> > > > There is a bug in vf clear xstats command, it do not record the
> > > > statics data in offset struct member.So, vf need to keep record of
> > > > xstats data from pf and update the statics according to offset.
> > > >
> > > > Fixes: da61cd0849766 ("i40evf: add extended stats")
> > > >
> > > > Signed-off-by: Wei Zhao
> > > >
> > > > ---
> > > >
> > > > Changes in v2:
> > > >
> > > > fix patch log check warning.
> > > > ---
> > > > app/test-pmd/config.c | 6 ++--
> > > > drivers/net/i40e/i40e_ethdev_vf.c | 64
> > > > ++-
> > > > 2 files changed, 67 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > > > 3ae3e1c..14131d6 100644
> > > > --- a/app/test-pmd/config.c
> > > > +++ b/app/test-pmd/config.c
> > > > @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> > > > if (diff_cycles > 0)
> > > > diff_cycles = prev_cycles[port_id] - diff_cycles;
> > > >
> > > > - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> > > > - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> > > > + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> > > > + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> > > > + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> > > > + (stats.opackets - prev_pkts_tx[port_id]) : 0;
> > >
> > > I guess this testpmd update is not directly related to this patch,
> > > but to protect testpmd against value overflow? Can this be another patch?
> >
> > Nonono, this code change is directly related to this patch, if we do
> > not do this code change, the diff_pkts_rx and diff_pkts_tx statistic
> > data will be wrong when the first time after clear xstats command.
> >
> Yes, the fix will make the error happen, but this is the fix of the clear
> xstats
> issue.
> You can create a separate patch for it just to make clearer.
OK, I will create a separate patch for it later.
>
> <..>
> > This bug only appear after use CLI "clear port xstats 0". So it is not
> > easy to detect this bug.
> > After using this fix patch ,the big user who report this issue has
> > feed back it work well now.
> > The root cause is not so complicated, when the pf which admin this vf
> > is in kernel state, DPDK can not Give pf the info to clear and update
> > offset command, so vf can only keep record the offset data in DPDK VF
> > port locally.
> >
> That was because i40evf PMD doesn't support the ops "stats_reset", but
> "xstats_reset"
> Is implemented when xstats are introduced in i40vf PMD.
> I think you also need to add the ops "stats_reset", testing can cover this
> basic
> case then.
Yes , I have create a new patch for add support of reset stats in vf port.
http://dpdk.org/dev/patchwork/patch/28046/
>
> Thanks
> Jingjing
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
> -Original Message-
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Zhao1, Wei
> Sent: Friday, September 1, 2017 10:30 AM
> To: Yigit, Ferruh ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
> Hi, Ferruh
>
> > -Original Message-
> > From: Yigit, Ferruh
> > Sent: Friday, September 1, 2017 12:54 AM
> > To: Zhao1, Wei ; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> > port
> >
> > On 8/29/2017 3:28 AM, Wei Zhao wrote:
> > > There is a bug in vf clear xstats command, it do not record the
> > > statics data in offset struct member.So, vf need to keep record of
> > > xstats data from pf and update the statics according to offset.
> > >
> > > Fixes: da61cd0849766 ("i40evf: add extended stats")
> > >
> > > Signed-off-by: Wei Zhao
> > >
> > > ---
> > >
> > > Changes in v2:
> > >
> > > fix patch log check warning.
> > > ---
> > > app/test-pmd/config.c | 6 ++--
> > > drivers/net/i40e/i40e_ethdev_vf.c | 64
> > > ++-
> > > 2 files changed, 67 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > > 3ae3e1c..14131d6 100644
> > > --- a/app/test-pmd/config.c
> > > +++ b/app/test-pmd/config.c
> > > @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> > > if (diff_cycles > 0)
> > > diff_cycles = prev_cycles[port_id] - diff_cycles;
> > >
> > > - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> > > - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> > > + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> > > + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> > > + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> > > + (stats.opackets - prev_pkts_tx[port_id]) : 0;
> >
> > I guess this testpmd update is not directly related to this patch, but to
> > protect
> > testpmd against value overflow? Can this be another patch?
>
> Nonono, this code change is directly related to this patch, if we do not do
> this code
> change, the
> diff_pkts_rx and diff_pkts_tx statistic data will be wrong when the first
> time after clear
> xstats command.
>
Yes, the fix will make the error happen, but this is the fix of the clear
xstats issue.
You can create a separate patch for it just to make clearer.
<..>
> This bug only appear after use CLI "clear port xstats 0". So it is not easy
> to detect this
> bug.
> After using this fix patch ,the big user who report this issue has feed back
> it work well
> now.
> The root cause is not so complicated, when the pf which admin this vf is in
> kernel state,
> DPDK can not
> Give pf the info to clear and update offset command, so vf can only keep
> record the
> offset data in DPDK
> VF port locally.
>
That was because i40evf PMD doesn't support the ops "stats_reset", but
"xstats_reset"
Is implemented when xstats are introduced in i40vf PMD.
I think you also need to add the ops "stats_reset", testing can cover this
basic case then.
Thanks
Jingjing
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
Hi, Ferruh
> -Original Message-
> From: Yigit, Ferruh
> Sent: Friday, September 1, 2017 12:54 AM
> To: Zhao1, Wei ; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf
> port
>
> On 8/29/2017 3:28 AM, Wei Zhao wrote:
> > There is a bug in vf clear xstats command, it do not record the
> > statics data in offset struct member.So, vf need to keep record of
> > xstats data from pf and update the statics according to offset.
> >
> > Fixes: da61cd0849766 ("i40evf: add extended stats")
> >
> > Signed-off-by: Wei Zhao
> >
> > ---
> >
> > Changes in v2:
> >
> > fix patch log check warning.
> > ---
> > app/test-pmd/config.c | 6 ++--
> > drivers/net/i40e/i40e_ethdev_vf.c | 64
> > ++-
> > 2 files changed, 67 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 3ae3e1c..14131d6 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> > if (diff_cycles > 0)
> > diff_cycles = prev_cycles[port_id] - diff_cycles;
> >
> > - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> > - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> > + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> > + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> > + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> > + (stats.opackets - prev_pkts_tx[port_id]) : 0;
>
> I guess this testpmd update is not directly related to this patch, but to
> protect
> testpmd against value overflow? Can this be another patch?
Nonono, this code change is directly related to this patch, if we do not do
this code change, the
diff_pkts_rx and diff_pkts_tx statistic data will be wrong when the first time
after clear xstats command.
>
> <...>
>
> > static int
> > i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats
> > *stats) {
> > int ret;
> > struct i40e_eth_stats *pstats = NULL;
> > + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data-
> >dev_private);
> > + struct i40e_vsi *vsi = &vf->vsi;
> >
> > ret = i40evf_update_stats(dev, &pstats);
> > if (ret != 0)
> > return 0;
> >
> > + i40evf_update_vsi_stats(vsi, pstats);
>
> But not having this previously means all VF stats were wrong previously, not
> only extended ones, also basic ones. And not not wrong with small
> difference, this should give a big difference in the stats.
>
> I am suspicious about this part, because if this is the case, I would expect
> this
> should be detected earlier.
>
> I have not traced the code, but is there any chance that "eth_stats_offset"
> has been used by other end of the admin command?
To be frankly speaking, this bug is firstly discovered by a big user.
This bug only appear after use CLI "clear port xstats 0". So it is not easy to
detect this bug.
After using this fix patch ,the big user who report this issue has feed back it
work well now.
The root cause is not so complicated, when the pf which admin this vf is in
kernel state, DPDK can not
Give pf the info to clear and update offset command, so vf can only keep record
the offset data in DPDK
VF port locally.
>
> > +
> > stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> > pstats->rx_broadcast;
> > stats->opackets = pstats->tx_broadcast + pstats->tx_multicast + @@
> > -1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
> > i40evf_update_stats(dev, &pstats);
> >
> > /* set stats offset base on current values */
> > - vf->vsi.eth_stats_offset = vf->vsi.eth_stats;
> > + vf->vsi.eth_stats_offset = *pstats;
>
> I can see this is the reason of the defect mentioned in the commit log.
> Instead of using newly acquired stats as offset, using old values...
>
> <...>
Re: [dpdk-dev] [PATCH v2 1/2] net/i40e: fix clear xstats bug in vf port
On 8/29/2017 3:28 AM, Wei Zhao wrote:
> There is a bug in vf clear xstats command, it do not
> record the statics data in offset struct member.So, vf
> need to keep record of xstats data from pf and update
> the statics according to offset.
>
> Fixes: da61cd0849766 ("i40evf: add extended stats")
>
> Signed-off-by: Wei Zhao
>
> ---
>
> Changes in v2:
>
> fix patch log check warning.
> ---
> app/test-pmd/config.c | 6 ++--
> drivers/net/i40e/i40e_ethdev_vf.c | 64
> ++-
> 2 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 3ae3e1c..14131d6 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -203,8 +203,10 @@ nic_stats_display(portid_t port_id)
> if (diff_cycles > 0)
> diff_cycles = prev_cycles[port_id] - diff_cycles;
>
> - diff_pkts_rx = stats.ipackets - prev_pkts_rx[port_id];
> - diff_pkts_tx = stats.opackets - prev_pkts_tx[port_id];
> + diff_pkts_rx = (stats.ipackets > prev_pkts_rx[port_id]) ?
> + (stats.ipackets - prev_pkts_rx[port_id]) : 0;
> + diff_pkts_tx = (stats.opackets > prev_pkts_tx[port_id]) ?
> + (stats.opackets - prev_pkts_tx[port_id]) : 0;
I guess this testpmd update is not directly related to this patch, but
to protect testpmd against value overflow? Can this be another patch?
<...>
> static int
> i40evf_get_statistics(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
> {
> int ret;
> struct i40e_eth_stats *pstats = NULL;
> + struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> + struct i40e_vsi *vsi = &vf->vsi;
>
> ret = i40evf_update_stats(dev, &pstats);
> if (ret != 0)
> return 0;
>
> + i40evf_update_vsi_stats(vsi, pstats);
But not having this previously means all VF stats were wrong previously,
not only extended ones, also basic ones. And not not wrong with small
difference, this should give a big difference in the stats.
I am suspicious about this part, because if this is the case, I would
expect this should be detected earlier.
I have not traced the code, but is there any chance that
"eth_stats_offset" has been used by other end of the admin command?
> +
> stats->ipackets = pstats->rx_unicast + pstats->rx_multicast +
> pstats->rx_broadcast;
> stats->opackets = pstats->tx_broadcast + pstats->tx_multicast +
> @@ -1025,7 +1083,7 @@ i40evf_dev_xstats_reset(struct rte_eth_dev *dev)
> i40evf_update_stats(dev, &pstats);
>
> /* set stats offset base on current values */
> - vf->vsi.eth_stats_offset = vf->vsi.eth_stats;
> + vf->vsi.eth_stats_offset = *pstats;
I can see this is the reason of the defect mentioned in the commit log.
Instead of using newly acquired stats as offset, using old values...
<...>
