[2/3] net/pcap: fix transmit return count in error conditions
Checks
Commit Message
When a packet cannot be transmitted, the driver is supposed to free this
packet and report it as handled.
This is to prevent the application from retrying to send the same packet
and ending up in a liveloop since the driver will never manage to send
it.
Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
Fixes: 6db141c91e1f ("pcap: support jumbo frames")
CC: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
Comments
On 7/24/2019 12:54 PM, David Marchand wrote:
> When a packet cannot be transmitted, the driver is supposed to free this
> packet and report it as handled.
> This is to prevent the application from retrying to send the same packet
> and ending up in a liveloop since the driver will never manage to send
> it.
>
> Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> CC: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index 470867d..5e5aab7 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -354,7 +354,8 @@ struct pmd_devargs_all {
> mbuf->pkt_len,
> RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>
> - break;
> + rte_pktmbuf_free(mbuf);
> + continue;
+1
Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
return value, but this looks better, to free the mbuf and record it as error.
> }
> }
>
> @@ -373,7 +374,7 @@ struct pmd_devargs_all {
> dumper_q->tx_stat.bytes += tx_bytes;
> dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
>
> - return num_tx;
> + return nb_pkts;
> }
>
> /*
> @@ -439,14 +440,15 @@ struct pmd_devargs_all {
> mbuf->pkt_len,
> RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>
> - break;
> + rte_pktmbuf_free(mbuf);
> + continue;
> }
> }
>
> - if (unlikely(ret != 0))
> - break;
> - num_tx++;
> - tx_bytes += mbuf->pkt_len;
> + if (ret == 0) {
> + num_tx++;
> + tx_bytes += mbuf->pkt_len;
> + }
I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
interfaces.
if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
cause a liveloop. Why not keep the existing behavior and let application to decide?
> rte_pktmbuf_free(mbuf);
> }
>
> @@ -454,7 +456,7 @@ struct pmd_devargs_all {
> tx_queue->tx_stat.bytes += tx_bytes;
> tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
>
> - return num_tx;
> + return nb_pkts;
> }
>
> /*
>
On Wed, Jul 24, 2019 at 8:36 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 7/24/2019 12:54 PM, David Marchand wrote:
> > When a packet cannot be transmitted, the driver is supposed to free this
> > packet and report it as handled.
> > This is to prevent the application from retrying to send the same packet
> > and ending up in a liveloop since the driver will never manage to send
> > it.
> >
> > Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> > Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> > CC: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> > index 470867d..5e5aab7 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -354,7 +354,8 @@ struct pmd_devargs_all {
> > mbuf->pkt_len,
> > RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > - break;
> > + rte_pktmbuf_free(mbuf);
> > + continue;
>
> +1
> Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
> return value, but this looks better, to free the mbuf and record it as error.
>
> > }
> > }
> >
> > @@ -373,7 +374,7 @@ struct pmd_devargs_all {
> > dumper_q->tx_stat.bytes += tx_bytes;
> > dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
> >
> > - return num_tx;
> > + return nb_pkts;
> > }
> >
> > /*
> > @@ -439,14 +440,15 @@ struct pmd_devargs_all {
> > mbuf->pkt_len,
> > RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > - break;
> > + rte_pktmbuf_free(mbuf);
> > + continue;
> > }
> > }
> >
> > - if (unlikely(ret != 0))
> > - break;
> > - num_tx++;
> > - tx_bytes += mbuf->pkt_len;
> > + if (ret == 0) {
> > + num_tx++;
> > + tx_bytes += mbuf->pkt_len;
> > + }
>
> I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
> interfaces.
>
> if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
> cause a liveloop. Why not keep the existing behavior and let application to decide?
The manual is not clear to me.
Do we really have temporary situations where retries are fine? and if
so, can we differentiate them from things like incorrect permissions
etc...
If we can't, dropping is safer.
On 7/25/2019 8:40 AM, David Marchand wrote:
> On Wed, Jul 24, 2019 at 8:36 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 7/24/2019 12:54 PM, David Marchand wrote:
>>> When a packet cannot be transmitted, the driver is supposed to free this
>>> packet and report it as handled.
>>> This is to prevent the application from retrying to send the same packet
>>> and ending up in a liveloop since the driver will never manage to send
>>> it.
>>>
>>> Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
>>> Fixes: 6db141c91e1f ("pcap: support jumbo frames")
>>> CC: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>> drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
>>> 1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
>>> index 470867d..5e5aab7 100644
>>> --- a/drivers/net/pcap/rte_eth_pcap.c
>>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>>> @@ -354,7 +354,8 @@ struct pmd_devargs_all {
>>> mbuf->pkt_len,
>>> RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>>>
>>> - break;
>>> + rte_pktmbuf_free(mbuf);
>>> + continue;
>>
>> +1
>> Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
>> return value, but this looks better, to free the mbuf and record it as error.
>>
>>> }
>>> }
>>>
>>> @@ -373,7 +374,7 @@ struct pmd_devargs_all {
>>> dumper_q->tx_stat.bytes += tx_bytes;
>>> dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
>>>
>>> - return num_tx;
>>> + return nb_pkts;
>>> }
>>>
>>> /*
>>> @@ -439,14 +440,15 @@ struct pmd_devargs_all {
>>> mbuf->pkt_len,
>>> RTE_ETHER_MAX_JUMBO_FRAME_LEN);
>>>
>>> - break;
>>> + rte_pktmbuf_free(mbuf);
>>> + continue;
>>> }
>>> }
>>>
>>> - if (unlikely(ret != 0))
>>> - break;
>>> - num_tx++;
>>> - tx_bytes += mbuf->pkt_len;
>>> + if (ret == 0) {
>>> + num_tx++;
>>> + tx_bytes += mbuf->pkt_len;
>>> + }
>>
>> I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
>> interfaces.
>>
>> if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
>> cause a liveloop. Why not keep the existing behavior and let application to decide?
>
> The manual is not clear to me.
> Do we really have temporary situations where retries are fine?
Same here, I am not that clear, I assume these situations exists, specially
taking into account nobody complaining about existing implementation.
> and if
> so, can we differentiate them from things like incorrect permissions
> etc...
>
> If we can't, dropping is safer.
>
May not differentiate them, because all we are getting is the fail from API
without reason.
But overall instead of driving freeing a packet, it feels better to leave this
decision to application unless driver needs to free it.
Other changes in the patchset is fixing defects in driver, only this part
changes behavior without a clear defect, I am leaning to keep old behavior.
@@ -354,7 +354,8 @@ struct pmd_devargs_all {
mbuf->pkt_len,
RTE_ETHER_MAX_JUMBO_FRAME_LEN);
- break;
+ rte_pktmbuf_free(mbuf);
+ continue;
}
}
@@ -373,7 +374,7 @@ struct pmd_devargs_all {
dumper_q->tx_stat.bytes += tx_bytes;
dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
- return num_tx;
+ return nb_pkts;
}
/*
@@ -439,14 +440,15 @@ struct pmd_devargs_all {
mbuf->pkt_len,
RTE_ETHER_MAX_JUMBO_FRAME_LEN);
- break;
+ rte_pktmbuf_free(mbuf);
+ continue;
}
}
- if (unlikely(ret != 0))
- break;
- num_tx++;
- tx_bytes += mbuf->pkt_len;
+ if (ret == 0) {
+ num_tx++;
+ tx_bytes += mbuf->pkt_len;
+ }
rte_pktmbuf_free(mbuf);
}
@@ -454,7 +456,7 @@ struct pmd_devargs_all {
tx_queue->tx_stat.bytes += tx_bytes;
tx_queue->tx_stat.err_pkts += nb_pkts - num_tx;
- return num_tx;
+ return nb_pkts;
}
/*