RE: [PATCH v3] devtools: ensure proper tag sequence
> -Original Message-
> From: jpali...@marvell.com
> Sent: Tuesday, July 19, 2022 2:01 PM
> To: Jakub Palider
> Cc: david.march...@redhat.com; dev@dpdk.org; tho...@monjalon.net
> Subject: [PATCH v3] devtools: ensure proper tag sequence
>
> From: Jakub Palider
>
> This change to log checking procedure ensures that certain tags are in proper
> order.
> The order of tags is as follows:
> * Coverity issue
> * Bugzilla ID
> * Fixes
> * Cc
> *
> * Suggested-by
> * Reported-by
> + Signed-off-by
> * Acked-by
> * Reviewed-by
> * Tested-by
> where:
> * => 0 or more than one instance possible + => more than once instance
> possible In order to satisfy the above requirements an extra check is
> performed for obligatory tags.
>
> v3:
> * restored some tags under check
> * defined chronological order after Signed-off
>
> v2:
> * narrowed down the tags under check
>
> Signed-off-by: Jakub Palider
> ---
> devtools/check-git-log.sh | 56 +
> doc/guides/contributing/patches.rst | 28 +++
> 2 files changed, 84 insertions(+)
>
> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh index
> 23c6a7d9bb..9b98207f3a 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -54,6 +54,7 @@ fixes=$(git log --format='%h %s' --reverse $range | grep
> -i ': *fix' | cut -d' '
> stablefixes=$($selfdir/git-log-fixes.sh $range | sed '/(N\/A)$/d' | cut -d'
> ' -
> f2) tags=$(git log --format='%b' --reverse $range | grep -i -e 'by *:' -e
> 'fix.*:')
> bytag='\(Reported\|Suggested\|Signed-off\|Acked\|Reviewed\|Tested\)-
> by:'
> +exttag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:'
>
> failure=false
>
> @@ -203,6 +204,61 @@ done)
> [ -z "$bad" ] || { printf "Is it candidate for Cc: sta...@dpdk.org
> backport?\n$bad\n"\
> && failure=true;}
>
> +# check tag sequence
> +bad=$(for commit in $commits; do
> + body=$(git log --format='%b' -1 $commit)
> + echo "$body" |\
> + grep -o -e "$exttag\|^[[:blank:]]*$\|$bytag" | \
> + # retrieve tags only
> + cut -f1 -d":" |\
> + # it is okay to have several tags of the same type but for processing
> + # we need to squash them
> + uniq |\
> + # make sure the tags are in the proper order as presented in SEQ
> + awk -v cmt="$commit" 'BEGIN{
> + SEQ[0] = "Coverity issue";
> + SEQ[1] = "Bugzilla ID";
> + SEQ[2] = "Fixes";
> + SEQ[3] = "Cc";
> + SEQ[4] = "^$";
> + SEQ[5] = "Suggested-by";
> + SEQ[6] = "Reported-by";
> + SEQ[7] = "Signed-off-by";
> + SEQ[8] = "Acked-by";
> + SEQ[9] = "Reviewed-by";
> + SEQ[10] = "Tested-by";
> + latest = 0;
> + chronological = 0;
> + }
> + {
> + for (seq = 0; seq < length(SEQ); seq++) {
> + if (chronological == 1)
> + continue;
> + if (match($0, SEQ[seq])) {
> + if (seq < latest) {
> + print "\tCommit " cmt " (" $0 ":)";
> + break;
> + } else {
> + latest = seq;
> + }
> + }
> + }
> + if (match($0, "Signed-off-by"))
> + chronological = 1;
> + }'
> +done)
> +[ -z "$bad" ] || { printf "Wrong tag order: \n$bad\n"\
> + && failure=true;}
> +
> +# check required tag
> +bad=$(for commit in $commits; do
> + body=$(git log --format='%b' -1 $commit)
> + echo $body | grep -q "Signed-off-by:" \
> + || echo "\tCommit" $commit "(Signed-off-by:)"
> + done)
> +[ -z "$bad" ] || { printf "Missing obligatory tag: \n$bad\n"\
> + && failure=true;}
> +
> total=$(echo "$commits" | wc -l)
> if $failure ; then
> printf "\nInvalid patch(es) found - checked $total patch"
> diff --git a/doc/guides/contributing/patches.rst
> b/doc/guides/contributing/patches.rst
> index bebcaf3925.
RE: [EXT] Re: [PATCH] devtools: ensure proper tag sequence
> -Original Message- > From: Thomas Monjalon > Sent: Sunday, June 12, 2022 10:29 PM > To: Jakub Palider > Cc: dev@dpdk.org; david.march...@redhat.com > Subject: [EXT] Re: [PATCH] devtools: ensure proper tag sequence > > External Email > > -- > 12/06/2022 16:23, jpali...@marvell.com: > > From: Jakub Palider > > > > This change to log checking procedure ensures that all > > tags are in proper order. > > The order of tags is as follows: > > * Coverity issue > > * Bugzilla ID > > * Fixes > > * Cc > > * > > * Suggested-by > > * Reported-by > > + Signed-off-by > > * Acked-by > > * Reviewed-by > > * Tested-by > > For the *-by sequence, the order is more chronological. > Ack/Review/Test can be in any order I think. > > Hi Thomas, Do I get it right that the lines after are fine in current state and the patch could take care of the first section only?
RE: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence
> -Original Message- > From: Thomas Monjalon > Sent: Wednesday, June 15, 2022 8:21 AM > To: Jakub Palider > Cc: dev@dpdk.org; david.march...@redhat.com > Subject: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence > > External Email > > -- > 14/06/2022 00:21, jpali...@marvell.com: > > + SEQ[0] = "Coverity issue"; > > + SEQ[1] = "Bugzilla ID"; > > + SEQ[2] = "Fixes"; > > + SEQ[3] = "Cc"; > > + SEQ[4] = "^$"; > > + SEQ[5] = "Suggested-by"; > > + SEQ[6] = "Reported-by"; > > + SEQ[7] = "Signed-off-by"; > > + latest = 0; > > Do you think you could check that Review, Ack and Test are added after the > first Signed-off? Ok, thank you for clarification. So any further Signed-offs need not to be in a particular sequence as long as the first one precedes Review/Ack/Test (these 3 in any order). > > > +Tag order > > +~ > > + > > +There is a pattern indicating how certain tags should relate to each other. > > + > > +Example of proper tag sequence:: > > + > > + Coverity issue: > > + Bugzilla ID: > > + Fixes: > > + Cc: > > + > > + Suggested-by: > > + Reported-by: > > + Signed-off-by: > > Given it is an example, you could add Reviewed-by, Acked-by and Tested-by. Sure, I will restore it. > > + > > +Between first and second tag section there is and empty line. > > + > > +While ``Signed-off-by:`` is an obligatory tag and must exists in each > > +commit, all other tags are optional. Any tag, as long as it is in > > +proper location to other adjacent tags (if present), may occur multiple > times. > > + > > +Other tags shall be laid out in a chronological order. > > Yes, after the first Signed-off-by. Side question: what about Sponsored-by, I noticed it appeared just recently?
[dpdk-dev] Meson configuration for pkg-config for drivers' shared objects
Hi,
I experience a problem while building out-of-tree application against drivers'
shared libraries from dpdk that are _not_ accessed by means of lib/ interface.
This looks a bit like a lack of a link between the framework and what certain
drivers deliver (rte_pmd API). Employing pkg-config in current shape does not
satisfy the dependencies.
Please, have a look at the example below which materializes this problem of an
app.c:
#include
#include
int main()
{
rte_ether_format_addr(NULL, 0, NULL);
rte_pmd_i40e_set_vf_mac_addr(0, 0, NULL);
}
I want to use two symbols, one from librte_net and one from i40e net rte pmd.
1. PLAIN compilation: complains about both of them, as expected:
$ ${CROSS}-g++ -march=armv8.1-a -o bin-app app.c
/tmp/ccUac7fS.o: In function `main':
app.c:(.text+0x114): undefined reference to `rte_ether_format_addr'
app.c:(.text+0x124): undefined reference to `rte_pmd_i40e_set_vf_mac_addr'
collect2: error: ld returned 1 exit status
2. PKGCONFIG asked to fill dependencies adds them for dpdk lib/ but i40e rte
pmd is still missing:
${CROSS}-g++ -march=armv8.1-a -o bin-app app.c $(pkg-config --libs libdpdk)
/tmp/ccYhiA3K.o: In function `main':
app.c:(.text+0x124): undefined reference to `rte_pmd_i40e_set_vf_mac_addr'
collect2: error: ld returned 1 exit status
3. WORKADOUND: which adds explicitly dependency for linker:
${CROSS}-g++ -march=armv8.1-a -o bin-app app.c $(pkg-config --libs libdpdk)
-lrte_net_i40e
The borderline is that I don't want to add them explicitly, but rather
pkg-config do this for me. This would be realized if driver's lib (which
actually gets built as .so) was added to meson's dpdk_libraries variable:
my_lib_name = '_'.join(['rte', class, name])
shared_lib = shared_library(my_lib_name)
dpdk_libraries = [shared_lib] + dpdk_libraries
In such case $(pkg-config --libs libdpdk) would satisfy all dependencies.
My questions are:
1. Is there a better approach to this problem than one mentioned by me? Am I
missing something?
2. Shall RTE PMD APIs of certain drivers be a part of dpdk default pkg-config
configuration?
I have learned there used to be libdpdk.so but this is no longer applicable,
also this is not a plugin-alike issue. So I hope I am not duplicating topics -
I could not find anything resembling this one.
Regards,
Jakub
[dpdk-dev] [PATCH] net/ena: prepare TSO offload calculation
While ENA can handle checksum calculations in almost all cases,
it cannot do so when DF bit in IPv4 header is not set,
that is DF=0, and TSO is requested. For that situation pseudo
header must be prepared manually.
Signed-off-by: Jakub Palider
---
drivers/net/ena/ena_ethdev.c | 29 +++--
drivers/net/ena/ena_ethdev.h | 1 +
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index e99bf29..060bed9 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1360,6 +1360,10 @@ static int eth_ena_dev_init(struct rte_eth_dev *eth_dev)
/* Set max MTU for this device */
adapter->max_mtu = get_feat_ctx.dev_attr.max_mtu;
+ /* set device support for TSO */
+ adapter->tso4_supported = get_feat_ctx.offload.tx &
+ ENA_ADMIN_FEATURE_OFFLOAD_DESC_TSO_IPV4_MASK;
+
/* Copy MAC address and point DPDK to it */
eth_dev->data->mac_addrs = (struct ether_addr *)adapter->mac_addr;
ether_addr_copy((struct ether_addr *)get_feat_ctx.dev_attr.mac_addr,
@@ -1585,13 +1589,20 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue,
struct rte_mbuf **rx_pkts,
}
static uint16_t
-eth_ena_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
+eth_ena_prep_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
uint16_t nb_pkts)
{
int32_t ret;
uint32_t i;
struct rte_mbuf *m;
+ struct ena_ring *tx_ring = (struct ena_ring *)(tx_queue);
+ struct ipv4_hdr *ip_hdr;
uint64_t ol_flags;
+ uint16_t frag_field;
+
+ /* ENA needs partial checksum for TSO packets only, skip early */
+ if (!tx_ring->adapter->tso4_supported)
+ return nb_pkts;
for (i = 0; i != nb_pkts; i++) {
m = tx_pkts[i];
@@ -1611,7 +1622,21 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue, struct
rte_mbuf **rx_pkts,
return i;
}
#endif
- /* ENA doesn't need different phdr cskum for TSO */
+
+ if (!(m->ol_flags & PKT_TX_IPV4))
+ continue;
+
+ ip_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
+m->l2_len);
+ frag_field = rte_be_to_cpu_16(ip_hdr->fragment_offset);
+ if (frag_field & IPV4_HDR_DF_FLAG)
+ continue;
+
+ /* In case we are supposed to TSO and have DF not set (DF=0)
+* hardware must be provided with partial checksum, otherwise
+* it will take care of necessary calculations.
+*/
+
ret = rte_net_intel_cksum_flags_prepare(m,
ol_flags & ~PKT_TX_TCP_SEG);
if (ret != 0) {
diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h
index 4c7edbb..dc3080f 100644
--- a/drivers/net/ena/ena_ethdev.h
+++ b/drivers/net/ena/ena_ethdev.h
@@ -162,6 +162,7 @@ struct ena_adapter {
u16 num_queues;
u16 max_mtu;
+ u8 tso4_supported;
int id_number;
char name[ENA_NAME_MAX_LEN];
--
1.9.1
[dpdk-dev] [PATCH] net/ena: solve problem with host attributes
The hardware may reject adding host_info in case support for
host_info is missing in the list of supported features. On the
other hand the list of supported featues may contain support
for the host_info - typical bootstrap problem.
This patch solves it by removing check against support for
host_info attribute and improves error handling by reacting
only to host attribute write failure to the hardware.
---
drivers/net/ena/base/ena_com.c | 16
drivers/net/ena/ena_ethdev.c | 21 -
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ena/base/ena_com.c b/drivers/net/ena/base/ena_com.c
index 88053e3..bd6f3c6 100644
--- a/drivers/net/ena/base/ena_com.c
+++ b/drivers/net/ena/base/ena_com.c
@@ -2590,19 +2590,11 @@ int ena_com_set_host_attributes(struct ena_com_dev
*ena_dev)
struct ena_com_admin_queue *admin_queue;
struct ena_admin_set_feat_cmd cmd;
struct ena_admin_set_feat_resp resp;
+ int ret;
- int ret = 0;
-
- if (unlikely(!ena_dev)) {
- ena_trc_err("%s : ena_dev is NULL\n", __func__);
- return ENA_COM_NO_DEVICE;
- }
-
- if (!ena_com_check_supported_feature_id(ena_dev,
- ENA_ADMIN_HOST_ATTR_CONFIG)) {
- ena_trc_warn("Set host attribute isn't supported\n");
- return ENA_COM_PERMISSION;
- }
+ /* Host attribute config is called before ena_com_get_dev_attr_feat
+* so ena_com can't check if the feature is supported.
+*/
memset(&cmd, 0x0, sizeof(cmd));
admin_queue = &ena_dev->admin_queue;
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index e99bf29..f3666e5 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -368,12 +368,9 @@ static void ena_config_host_info(struct ena_com_dev
*ena_dev)
rc = ena_com_set_host_attributes(ena_dev);
if (rc) {
- if (rc == -EPERM)
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
- else
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
-
- goto err;
+ RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
+ if (rc != -EPERM)
+ goto err;
}
return;
@@ -424,11 +421,9 @@ static void ena_config_debug_area(struct ena_adapter
*adapter)
rc = ena_com_set_host_attributes(&adapter->ena_dev);
if (rc) {
- if (rc == -EPERM)
- RTE_LOG(WARNING, PMD, "Cannot set host attributes\n");
- else
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
- goto err;
+ RTE_LOG(WARNING, PMD, "Cannot set host attributes\n");
+ if (rc != -EPERM)
+ goto err;
}
return;
@@ -1239,14 +1234,14 @@ static int ena_device_init(struct ena_com_dev *ena_dev,
goto err_mmio_read_less;
}
- ena_config_host_info(ena_dev);
-
/* To enable the msix interrupts the driver needs to know the number
* of queues. So the driver uses polling mode to retrieve this
* information.
*/
ena_com_set_admin_polling_mode(ena_dev, true);
+ ena_config_host_info(ena_dev);
+
/* Get Device Attributes and features */
rc = ena_com_get_dev_attr_feat(ena_dev, get_feat_ctx);
if (rc) {
--
2.5.0
[dpdk-dev] [PATCH v2] net/ena: solve problem with host attributes
The hardware may reject adding host_info in case support for
host_info is missing in the list of supported features. On the
other hand the list of supported featues may contain support
for the host_info - typical bootstrap problem.
This patch solves it by removing check against support for
host_info attribute and improves error handling by reacting
only to host attribute write failure to the hardware.
v2 changes:
- added Signed-off
Signed-off-by: Jakub Palider
---
drivers/net/ena/base/ena_com.c | 16
drivers/net/ena/ena_ethdev.c | 21 -
2 files changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ena/base/ena_com.c b/drivers/net/ena/base/ena_com.c
index 88053e3..bd6f3c6 100644
--- a/drivers/net/ena/base/ena_com.c
+++ b/drivers/net/ena/base/ena_com.c
@@ -2590,19 +2590,11 @@ int ena_com_set_host_attributes(struct ena_com_dev
*ena_dev)
struct ena_com_admin_queue *admin_queue;
struct ena_admin_set_feat_cmd cmd;
struct ena_admin_set_feat_resp resp;
+ int ret;
- int ret = 0;
-
- if (unlikely(!ena_dev)) {
- ena_trc_err("%s : ena_dev is NULL\n", __func__);
- return ENA_COM_NO_DEVICE;
- }
-
- if (!ena_com_check_supported_feature_id(ena_dev,
- ENA_ADMIN_HOST_ATTR_CONFIG)) {
- ena_trc_warn("Set host attribute isn't supported\n");
- return ENA_COM_PERMISSION;
- }
+ /* Host attribute config is called before ena_com_get_dev_attr_feat
+* so ena_com can't check if the feature is supported.
+*/
memset(&cmd, 0x0, sizeof(cmd));
admin_queue = &ena_dev->admin_queue;
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index e99bf29..f3666e5 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -368,12 +368,9 @@ static void ena_config_host_info(struct ena_com_dev
*ena_dev)
rc = ena_com_set_host_attributes(ena_dev);
if (rc) {
- if (rc == -EPERM)
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
- else
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
-
- goto err;
+ RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
+ if (rc != -EPERM)
+ goto err;
}
return;
@@ -424,11 +421,9 @@ static void ena_config_debug_area(struct ena_adapter
*adapter)
rc = ena_com_set_host_attributes(&adapter->ena_dev);
if (rc) {
- if (rc == -EPERM)
- RTE_LOG(WARNING, PMD, "Cannot set host attributes\n");
- else
- RTE_LOG(ERR, PMD, "Cannot set host attributes\n");
- goto err;
+ RTE_LOG(WARNING, PMD, "Cannot set host attributes\n");
+ if (rc != -EPERM)
+ goto err;
}
return;
@@ -1239,14 +1234,14 @@ static int ena_device_init(struct ena_com_dev *ena_dev,
goto err_mmio_read_less;
}
- ena_config_host_info(ena_dev);
-
/* To enable the msix interrupts the driver needs to know the number
* of queues. So the driver uses polling mode to retrieve this
* information.
*/
ena_com_set_admin_polling_mode(ena_dev, true);
+ ena_config_host_info(ena_dev);
+
/* Get Device Attributes and features */
rc = ena_com_get_dev_attr_feat(ena_dev, get_feat_ctx);
if (rc) {
--
2.5.0
[dpdk-dev] [PATCH] net/ena: use correct field for Rx offload features
Previously the capability bitmap for Rx offloads was mistakenly taken from Tx capability bitmap field. This patch fixes the problem. Signed-off-by: Jakub Palider --- drivers/net/ena/ena_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c index dcee8ed..34e5577 100644 --- a/drivers/net/ena/ena_ethdev.c +++ b/drivers/net/ena/ena_ethdev.c @@ -1466,7 +1466,7 @@ static void ena_infos_get(struct rte_eth_dev *dev, DEV_TX_OFFLOAD_UDP_CKSUM | DEV_TX_OFFLOAD_TCP_CKSUM; - if (feat.offload.tx & + if (feat.offload.rx_supported & ENA_ADMIN_FEATURE_OFFLOAD_DESC_RX_L4_IPV4_CSUM_MASK) rx_feat |= DEV_RX_OFFLOAD_IPV4_CKSUM | DEV_RX_OFFLOAD_UDP_CKSUM | -- 2.5.0
[dpdk-dev] [PATCH 0/2] net/ena: check for free descriptors
In some configurations there is mismatch between declared and actual descriptor count which under heavy load may lead to resource shortage. The first patch unifies the way head and tail indexes are handled and is crucial for compactness and succeeding patch correctness. The second patch runs check for available descriptor count. Jakub Palider (2): net/ena: use unmasked head/tail values net/ena: check for free buffers prior to xmit drivers/net/ena/ena_ethdev.c | 83 ++-- drivers/net/ena/ena_ethdev.h | 24 ++--- 2 files changed, 52 insertions(+), 55 deletions(-) -- 2.5.0
[dpdk-dev] [PATCH 1/2] net/ena: use unmasked head/tail values
The next_to_clean and next_to_use ring pointers sometimes were kept
wrapped around ring size, sometimes used beyond this limit. From now
on we increment them without regard to ring size limits, but when
reading the values they are wrapped accordingly. Moreover unit16_t
are unsed whenever possible.
Signed-off-by: Tal Avraham
Signed-off-by: Jakub Palider
---
drivers/net/ena/ena_ethdev.c | 77 ++--
drivers/net/ena/ena_ethdev.h | 24 ++
2 files changed, 47 insertions(+), 54 deletions(-)
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index 737ae87..adf94f2 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -676,8 +676,7 @@ static void ena_rx_queue_release_bufs(struct ena_ring *ring)
if (m)
__rte_mbuf_raw_free(m);
- ring->next_to_clean =
- ENA_CIRC_INC(ring->next_to_clean, 1, ring->ring_size);
+ ring->next_to_clean++;
}
}
@@ -692,8 +691,7 @@ static void ena_tx_queue_release_bufs(struct ena_ring *ring)
if (tx_buf->mbuf)
rte_pktmbuf_free(tx_buf->mbuf);
- ring->next_to_clean =
- ENA_CIRC_INC(ring->next_to_clean, 1, ring->ring_size);
+ ring->next_to_clean++;
}
}
@@ -926,8 +924,8 @@ static int ena_queue_restart(struct ena_ring *ring)
if (ring->type == ENA_RING_TYPE_TX)
return 0;
- rc = ena_populate_rx_queue(ring, ring->ring_size - 1);
- if ((unsigned int)rc != ring->ring_size - 1) {
+ rc = ena_populate_rx_queue(ring, ring->ring_size);
+ if ((unsigned int)rc != ring->ring_size) {
PMD_INIT_LOG(ERR, "Failed to populate rx ring !\n");
return (-1);
}
@@ -962,6 +960,13 @@ static int ena_tx_queue_setup(struct rte_eth_dev *dev,
return -1;
}
+ if (!rte_is_power_of_2(nb_desc)) {
+ RTE_LOG(ERR, PMD,
+ "Unsupported size of RX queue: %d is not a power of 2.",
+ nb_desc);
+ return -EINVAL;
+ }
+
if (nb_desc > adapter->tx_ring_size) {
RTE_LOG(ERR, PMD,
"Unsupported size of TX queue (max size: %d)\n",
@@ -1056,6 +1061,13 @@ static int ena_rx_queue_setup(struct rte_eth_dev *dev,
return -1;
}
+ if (!rte_is_power_of_2(nb_desc)) {
+ RTE_LOG(ERR, PMD,
+ "Unsupported size of TX queue: %d is not a power of 2.",
+ nb_desc);
+ return -EINVAL;
+ }
+
if (nb_desc > adapter->rx_ring_size) {
RTE_LOG(ERR, PMD,
"Unsupported size of RX queue (max size: %d)\n",
@@ -1115,23 +1127,25 @@ static int ena_populate_rx_queue(struct ena_ring *rxq,
unsigned int count)
{
unsigned int i;
int rc;
- unsigned int ring_size = rxq->ring_size;
- unsigned int ring_mask = ring_size - 1;
- int next_to_use = rxq->next_to_use & ring_mask;
+ uint16_t ring_size = rxq->ring_size;
+ uint16_t ring_mask = ring_size - 1;
+ uint16_t next_to_use = rxq->next_to_use;
+ uint16_t in_use;
struct rte_mbuf **mbufs = &rxq->rx_buffer_info[0];
if (unlikely(!count))
return 0;
- ena_assert_msgENA_CIRC_COUNT(rxq->next_to_use, rxq->next_to_clean,
-rxq->ring_size)) +
-count) < rxq->ring_size), "bad ring state");
+ in_use = rxq->next_to_use - rxq->next_to_clean;
+ ena_assert_msg(((in_use + count) <= ring_size), "bad ring state");
- count = RTE_MIN(count, ring_size - next_to_use);
+ count = RTE_MIN(count,
+ (uint16_t)(ring_size - (next_to_use & ring_mask)));
/* get resources for incoming packets */
rc = rte_mempool_get_bulk(rxq->mb_pool,
- (void **)(&mbufs[next_to_use]), count);
+ (void **)(&mbufs[next_to_use & ring_mask]),
+ count);
if (unlikely(rc < 0)) {
rte_atomic64_inc(&rxq->adapter->drv_stats->rx_nombuf);
PMD_RX_LOG(DEBUG, "there are no enough free buffers");
@@ -1139,7 +1153,8 @@ static int ena_populate_rx_queue(struct ena_ring *rxq,
unsigned int count)
}
for (i = 0; i < count; i++) {
- struct rte_mbuf *mbuf = mbufs[next_to_use];
+ uint16_t next_to_use_masked = next_to_use & ring_mask;
+ struct rte_mbuf *mbuf = mbufs[next_t
[dpdk-dev] [PATCH 2/2] net/ena: check for free buffers prior to xmit
Signed-off-by: Tal Avraham
Signed-off-by: Jakub Palider
---
drivers/net/ena/ena_ethdev.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
index adf94f2..ab9a178 100644
--- a/drivers/net/ena/ena_ethdev.c
+++ b/drivers/net/ena/ena_ethdev.c
@@ -1583,7 +1583,7 @@ static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct
rte_mbuf **tx_pkts,
struct ena_tx_buffer *tx_info;
struct ena_com_buf *ebuf;
uint16_t rc, req_id, total_tx_descs = 0;
- uint16_t sent_idx = 0;
+ uint16_t sent_idx = 0, empty_tx_reqs;
int nb_hw_desc;
/* Check adapter state */
@@ -1593,6 +1593,10 @@ static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct
rte_mbuf **tx_pkts,
return 0;
}
+ empty_tx_reqs = ring_size - (next_to_use - next_to_clean);
+ if (nb_pkts > empty_tx_reqs)
+ nb_pkts = empty_tx_reqs;
+
for (sent_idx = 0; sent_idx < nb_pkts; sent_idx++) {
mbuf = tx_pkts[sent_idx];
--
2.5.0
Re: [dpdk-dev] [PATCH v2 0/4] Ena PMD fixes
On Mon, Apr 10, 2017 at 4:28 PM, Marcin Wojtas wrote: > Hi, > > I sent second version of a patchset with various fixes for ena PMD. > All remarks after v1 review have been taken into account. Details > can be found in the changelog below. > > We are looking forward to any comments or remarks. > > Best regards, > Marcin > > Changelog: > v1 -> v2 > * 1/4: > - Part of the changes related to the allocation of wrong amount of Rx > descriptors from patch 2 were moved to patch 1 > * 2/4: > - Remove additional variable desc_to_refill > - Update desc_to_use after reading all descriptors - significant diff > reduction > - Update information why next_to_use must be assigned before call of > the > ena_rx_populate() > - Update commit message to reflect recent changes > * 4/4: > - Check for the type of the packet before doing further investigation > in TSO > > Michal Krawczyk (4): > net/ena: fix incorrect Rx descriptors allocation > net/ena: fix delayed cleanup of Rx descriptors > net/ena: cleanup if refilling of rx descriptors fails > net/ena: calculate partial checksum if DF bit is disabled > > drivers/net/ena/ena_ethdev.c | 52 -- > -- > 1 file changed, 33 insertions(+), 19 deletions(-) > > -- > 1.8.3.1 > > Reviewed-by: Jakub Palider
Re: [dpdk-dev] [PATCH 1/4] net/ena: fix incorrect Rx descriptors allocation
Looks good to me but please, check commnt on patch 2/4. Moving the "-1"
indexing fix from patch 2/4 into 1/4 will reflect all related changes in
one place. But this is not a functional change, so may go in current shape
as well.
Jakub
On Fri, Apr 7, 2017 at 12:48 PM, Marcin Wojtas wrote:
> From: Michal Krawczyk
>
> When application tried to allocate 1024 descriptors, device was not
> initializing properly.
>
> This patch solves it by avoiding allocation of all descriptors in the ring
> in one attempt. At least one descriptor must remain unused in the HW ring.
>
> Fixes: 1173fca25af9 ("ena: add polling-mode driver")
>
> Signed-off-by: Michal Krawczyk
> ---
> drivers/net/ena/ena_ethdev.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index b5e6db6..2345bab 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -919,7 +919,7 @@ static int ena_start(struct rte_eth_dev *dev)
>
> static int ena_queue_restart(struct ena_ring *ring)
> {
> - int rc;
> + int rc, bufs_num;
>
> ena_assert_msg(ring->configured == 1,
>"Trying to restart unconfigured queue\n");
> @@ -930,8 +930,9 @@ static int ena_queue_restart(struct ena_ring *ring)
> if (ring->type == ENA_RING_TYPE_TX)
> return 0;
>
> - rc = ena_populate_rx_queue(ring, ring->ring_size);
> - if ((unsigned int)rc != ring->ring_size) {
> + bufs_num = ring->ring_size - 1;
> + rc = ena_populate_rx_queue(ring, bufs_num);
> + if (rc != bufs_num) {
> PMD_INIT_LOG(ERR, "Failed to populate rx ring !");
> return (-1);
> }
> --
> 1.8.3.1
>
>
Re: [dpdk-dev] [PATCH 2/4] net/ena: fix delayed cleanup of Rx descriptors
On Fri, Apr 7, 2017 at 12:48 PM, Marcin Wojtas wrote:
> From: Michal Krawczyk
>
> On RX path, after receiving bunch of packets, variable tracking
> available descriptors in HW queue was not updated.
>
> To fix this issue, additional variable was added which is storing number
> of depleted descriptors updated by number of descriptors used in this
> cycle.
>
> Finally whole number is substracted by one to do not refill all
> descriptors what is required by the driver.
>
> Fixes: 1daff5260ff8 ("net/ena: use unmasked head and tail")
>
> Signed-off-by: Michal Krawczyk
> ---
> drivers/net/ena/ena_ethdev.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 2345bab..d875a2b 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -1144,7 +1144,7 @@ static int ena_populate_rx_queue(struct ena_ring
> *rxq, unsigned int count)
> return 0;
>
> in_use = rxq->next_to_use - rxq->next_to_clean;
> - ena_assert_msg(((in_use + count) <= ring_size), "bad ring state");
> + ena_assert_msg(((in_use + count) < ring_size), "bad ring state");
>
> count = RTE_MIN(count,
> (uint16_t)(ring_size - (next_to_use & ring_mask)));
> @@ -1504,7 +1504,7 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue,
> struct rte_mbuf **rx_pkts,
> unsigned int ring_size = rx_ring->ring_size;
> unsigned int ring_mask = ring_size - 1;
> uint16_t next_to_clean = rx_ring->next_to_clean;
> - uint16_t desc_in_use = 0;
> + uint16_t desc_in_use, desc_to_refill;
> unsigned int recv_idx = 0;
> struct rte_mbuf *mbuf = NULL;
> struct rte_mbuf *mbuf_head = NULL;
> @@ -1575,12 +1575,13 @@ static uint16_t eth_ena_recv_pkts(void *rx_queue,
> struct rte_mbuf **rx_pkts,
> recv_idx++;
> }
>
> - /* Burst refill to save doorbells, memory barriers, const interval
> */
> - if (ring_size - desc_in_use > ENA_RING_DESCS_RATIO(ring_size))
> - ena_populate_rx_queue(rx_ring, ring_size - desc_in_use);
> -
> rx_ring->next_to_clean = next_to_clean;
>
> + desc_to_refill = ring_size - desc_in_use + completed - 1;
> + /* Burst refill to save doorbells, memory barriers, const interval
> */
> + if (desc_to_refill > ENA_RING_DESCS_RATIO(ring_size))
> + ena_populate_rx_queue(rx_ring, desc_to_refill);
> +
> return recv_idx;
> }
>
> --
> 1.8.3.1
>
>
Good catch! I would suggest, however, to slightly rework this this way:
adjust the desc_in_use to reflect the actual state:
desc_in_use -= completed;
so that it would be the only change in this patch, and should do the trick.
As for the adjustment "-1" I will refer to 1/4 separately.
Jakub
