You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/11/18 22:11:17 UTC
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7616: net/l2/l3/l4: add support of iob offload
pkarashchenko commented on code in PR #7616:
URL: https://github.com/apache/incubator-nuttx/pull/7616#discussion_r1026396873
##########
arch/sim/src/sim/up_netdriver.c:
##########
@@ -402,22 +436,21 @@ int netdriver_init(void)
netdriver_txdone_interrupt,
netdriver_rxready_interrupt);
- /* Update the buffer size */
-
- pktsize = dev->d_pktsize ? dev->d_pktsize :
- (MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE);
-
/* Allocate packet buffer */
- pktbuf = kmm_malloc(pktsize);
- if (pktbuf == NULL)
+#ifdef SIM_NETDEV_IOB_OFFLOAD
+ dev->d_buf = NULL;
+#else
+ dev->d_buf = kmm_malloc(dev->d_pktsize ?
Review Comment:
Optional
```suggestion
dev->d_buf = kmm_malloc(dev->d_pktsize != 0 ?
```
##########
arch/sim/src/sim/up_netdriver.c:
##########
@@ -107,6 +120,10 @@ static void netdriver_reply(struct net_driver_s *dev)
if (dev->d_len > 0)
{
+#ifdef SIM_NETDEV_IOB_OFFLOAD
+ dev->d_buf = (void *)ETHBUF;
Review Comment:
do we need `void *` cast?
##########
net/icmp/icmp_recvmsg.c:
##########
@@ -227,79 +227,35 @@ static inline ssize_t icmp_readahead(FAR struct icmp_conn_s *conn,
FAR struct sockaddr_in *from,
FAR socklen_t *fromlen)
{
- FAR struct sockaddr_in bitbucket;
FAR struct iob_s *iob;
ssize_t ret = -ENODATA;
- int recvlen;
/* Check there is any ICMP replies already buffered in a read-ahead
* buffer.
*/
if ((iob = iob_peek_queue(&conn->readahead)) != NULL)
{
- FAR struct iob_s *tmp;
- uint16_t offset;
- uint8_t addrsize;
-
DEBUGASSERT(iob->io_pktlen > 0);
- /* Transfer that buffered data from the I/O buffer chain into
- * the user buffer.
- */
-
- /* First get the size of the address */
-
- recvlen = iob_copyout(&addrsize, iob, sizeof(uint8_t), 0);
- if (recvlen != sizeof(uint8_t))
- {
- ret = -EIO;
- goto out;
- }
-
- offset = sizeof(uint8_t);
-
- if (addrsize > sizeof(struct sockaddr_in))
- {
- ret = -EINVAL;
- goto out;
- }
-
/* Then get address */
- if (from == NULL)
- {
- from = &bitbucket;
- }
-
- recvlen = iob_copyout((FAR uint8_t *)from, iob, addrsize, offset);
- if (recvlen != addrsize)
+ if (from)
Review Comment:
```suggestion
if (from != NULL)
```
##########
net/devif/devif_iobsend.c:
##########
@@ -53,24 +53,65 @@
****************************************************************************/
void devif_iob_send(FAR struct net_driver_s *dev, FAR struct iob_s *iob,
- unsigned int len, unsigned int offset)
+ unsigned int len, unsigned int offset,
+ unsigned int skip)
{
+ unsigned int copyin;
+ int ret;
+
if (dev == NULL || len == 0 || len >= NETDEV_PKTSIZE(dev))
{
- nerr("devif_iob_send error, %p, send len: %u, pkt len: %u\n",
- dev, len, NETDEV_PKTSIZE(dev));
+ nerr("%s error, %p, send len: %u, pkt len: %u\n",
+ __func__, dev, len, NETDEV_PKTSIZE(dev));
return;
}
- /* Copy the data from the I/O buffer chain to the device buffer */
+ /* Append the send buffer after device buffer */
+
+ if (dev->d_iob != NULL)
+ {
+ /* Skip the l3/l4 offset before append */
+
+ netdev_iob_update(dev->d_iob, dev->d_iob->io_offset, skip);
+
+ dev->d_sndlen = len;
+
+ /* Clone the iob to target device buffer */
- iob_copyout(dev->d_appdata, iob, len, offset);
- dev->d_sndlen = len;
+ while (iob && len > 0)
+ {
+ copyin = (len > iob->io_len) ? iob->io_len : len;
+
+ ret = iob_copyin(dev->d_iob, iob->io_data,
+ copyin, skip, false);
+ if (ret != copyin)
+ {
+ netdev_iob_release(dev);
+ dev->d_sndlen = 0;
+ nerr("%s error, not enough iob entries, "
+ "send len: %u\n",
+ __func__, len);
+ return;
+ }
+
+ skip += copyin;
+ len -= copyin;
+ iob = iob->io_flink;
+ }
+ }
+ else
+ {
+ /* Send the iob directly if no device buffer */
+
+ dev->d_iob = iob;
+ dev->d_sndlen = len;
+ dev->d_buf = (FAR void *)ETHBUF;
+ }
#ifdef CONFIG_NET_TCP_WRBUFFER_DUMP
/* Dump the outgoing device buffer */
- lib_dumpbuffer("devif_iob_send", dev->d_appdata, len);
+ lib_dumpbuffer(__func__, dev->d_appdata, len);
Review Comment:
Ditto
##########
net/devif/devif_poll.c:
##########
@@ -639,15 +698,44 @@ static inline int devif_poll_tcp_connections(FAR struct net_driver_s *dev,
* field is set to a value larger than zero. The device driver should then
* send out the packet.
*
+ * This is the iob buffer version of devif_input(),
+ * this function will support send/receive iob vectors directly between
+ * the driver and l3/l4 stack to avoid unnecessary memory copies,
+ * especially on hardware that supports Scatter/gather, which can
+ * greatly improve performance
+ * this function will uses d_iob as packets input which used by some
+ * NICs such as celluler net driver.
+ *
+ * If NIC hardware support Scatter/gather transfer
+ *
+ * tcp_poll()/udp_poll()/pkt_poll()/...(l3/l4)
+ * / \
+ * / \
+ * devif_poll_[l3|l4]_connections() devif_iob_send() (nocopy:udp/icmp/...)
+ * / \ (copy:tcp)
+ * / \
+ * devif_iob_poll("NIC"_txpoll) callback() // "NIC"_txpoll
+ *
+ *
* Assumptions:
* This function is called from the MAC device driver with the network
* locked.
*
****************************************************************************/
-int devif_poll(FAR struct net_driver_s *dev, devif_poll_callback_t callback)
+int devif_iob_poll(FAR struct net_driver_s *dev,
Review Comment:
Can return type be changed to `bool`?
##########
net/icmp/icmp_input.c:
##########
@@ -100,86 +100,39 @@ static uint16_t icmp_datahandler(FAR struct net_driver_s *dev,
FAR struct ipv4_hdr_s *ipv4;
struct sockaddr_in inaddr;
FAR struct iob_s *iob;
- uint16_t offset;
- uint16_t buflen;
uint16_t iphdrlen;
- uint8_t addrsize;
+ uint16_t buflen;
int ret;
- /* Try to allocate on I/O buffer to start the chain without waiting (and
- * throttling as necessary). If we would have to wait, then drop the
- * packet.
- */
-
- iob = iob_tryalloc(true);
- if (iob == NULL)
- {
- nerr("ERROR: Failed to create new I/O buffer chain\n");
- goto drop;
- }
-
/* Put the IPv4 address at the beginning of the read-ahead buffer */
- ipv4 = IPv4BUF;
-
+ iob = dev->d_iob;
+ ipv4 = IPv4BUF;
inaddr.sin_family = AF_INET;
inaddr.sin_port = 0;
net_ipv4addr_copy(inaddr.sin_addr.s_addr,
net_ip4addr_conv32(ipv4->srcipaddr));
memset(inaddr.sin_zero, 0, sizeof(inaddr.sin_zero));
- /* Copy the src address info into the I/O buffer chain. We will not wait
- * for an I/O buffer to become available in this context. It there is
- * any failure to allocated, the entire I/O buffer chain will be discarded.
- */
-
- addrsize = sizeof(struct sockaddr_in);
- ret = iob_trycopyin(iob, &addrsize, sizeof(uint8_t), 0, true);
- if (ret < 0)
- {
- /* On a failure, iob_trycopyin return a negated error value but does
- * not free any I/O buffers.
- */
-
- nerr("ERROR: Failed to length to the I/O buffer chain: %d\n", ret);
- goto drop_with_chain;
- }
-
- offset = sizeof(uint8_t);
-
- ret = iob_trycopyin(iob, (FAR const uint8_t *)&inaddr,
- sizeof(struct sockaddr_in), offset, true);
- if (ret < 0)
- {
- /* On a failure, iob_trycopyin return a negated error value but does
- * not free any I/O buffers.
- */
-
- nerr("ERROR: Failed to source address to the I/O buffer chain: %d\n",
- ret);
- goto drop_with_chain;
- }
-
- offset += sizeof(struct sockaddr_in);
-
/* Get the IP header length (accounting for possible options). */
iphdrlen = (ipv4->vhl & IPv4_HLMASK) << 2;
+ /* Copy the src address info into the front of I/O buffer chain which
+ * overwrites the contents of the packet header field.
+ */
+
+ memcpy(iob->io_data, (FAR const uint8_t *)&inaddr,
Review Comment:
Is type cast needed here?
##########
net/udp/udp_sendto_buffered.c:
##########
@@ -327,6 +333,46 @@ static int sendto_next_transfer(FAR struct udp_conn_s *conn)
return OK;
}
+/****************************************************************************
+ * Name: send_tcpip_hdrsize
+ *
+ * Description:
+ * Get the total size of L3 and L4 UDP header
+ *
+ * Input Parameters:
+ * conn The connection structure associated with the socket
+ *
+ * Returned Value:
+ * the total size of L3 and L4 TCP header
+ *
+ ****************************************************************************/
+
+static inline uint16_t sendto_udpip_hdrsize(FAR struct udp_conn_s *conn)
+{
+#if defined(CONFIG_NET_IPv4) && defined(CONFIG_NET_IPv6)
+ /* Which domain the socket used */
+
+ if (conn->domain == PF_INET)
+ {
+ /* Select the IPv4 domain */
+
+ return sizeof(struct ipv4_hdr_s) + sizeof(struct udp_hdr_s);
+ }
+ else /* if (domain == PF_INET6) */
+ {
+ /* Select the IPv6 domain */
+
+ return sizeof(struct ipv6_hdr_s) + sizeof(struct udp_hdr_s);
+ }
+#elif defined(CONFIG_NET_IPv4)
+ ((void)conn);
Review Comment:
Ditto
##########
arch/sim/src/sim/up_netdriver.c:
##########
@@ -72,6 +72,19 @@
#include "up_internal.h"
+#if CONFIG_IOB_BUFSIZE > (MAX_NETDEV_PKTSIZE + \
+ CONFIG_NET_GUARDSIZE + \
+ CONFIG_NET_LL_GRUARDSIZE)
+# define SIM_NETDEV_IOB_OFFLOAD
+#endif
+
+#ifdef SIM_NETDEV_IOB_OFFLOAD
+# define devif_poll devif_iob_poll
+# define ipv4_input ipv4_iob_input
+# define ipv6_input ipv6_iob_input
+# define pkt_input pkt_iob_input
Review Comment:
```suggestion
# define devif_poll devif_iob_poll
# define ipv4_input ipv4_iob_input
# define ipv6_input ipv6_iob_input
# define pkt_input pkt_iob_input
```
##########
net/devif/devif_iobsend.c:
##########
@@ -53,24 +53,65 @@
****************************************************************************/
void devif_iob_send(FAR struct net_driver_s *dev, FAR struct iob_s *iob,
- unsigned int len, unsigned int offset)
+ unsigned int len, unsigned int offset,
+ unsigned int skip)
{
+ unsigned int copyin;
+ int ret;
+
if (dev == NULL || len == 0 || len >= NETDEV_PKTSIZE(dev))
{
- nerr("devif_iob_send error, %p, send len: %u, pkt len: %u\n",
- dev, len, NETDEV_PKTSIZE(dev));
+ nerr("%s error, %p, send len: %u, pkt len: %u\n",
+ __func__, dev, len, NETDEV_PKTSIZE(dev));
return;
}
- /* Copy the data from the I/O buffer chain to the device buffer */
+ /* Append the send buffer after device buffer */
+
+ if (dev->d_iob != NULL)
+ {
+ /* Skip the l3/l4 offset before append */
+
+ netdev_iob_update(dev->d_iob, dev->d_iob->io_offset, skip);
+
+ dev->d_sndlen = len;
+
+ /* Clone the iob to target device buffer */
- iob_copyout(dev->d_appdata, iob, len, offset);
- dev->d_sndlen = len;
+ while (iob && len > 0)
Review Comment:
```suggestion
while (iob != NULL && len > 0)
```
##########
mm/iob/Kconfig:
##########
@@ -41,7 +41,7 @@ config IOB_HEADSIZE
config IOB_ALIGNMENT
int "Alignment size of each I/O buffer"
- default 1
+ default 8
Review Comment:
I think taking into account specific of a NuttX network subsystem the default alignment should be 2.
##########
arch/sim/src/sim/up_netdriver.c:
##########
@@ -72,6 +72,19 @@
#include "up_internal.h"
+#if CONFIG_IOB_BUFSIZE > (MAX_NETDEV_PKTSIZE + \
+ CONFIG_NET_GUARDSIZE + \
+ CONFIG_NET_LL_GRUARDSIZE)
+# define SIM_NETDEV_IOB_OFFLOAD
Review Comment:
```suggestion
# define SIM_NETDEV_IOB_OFFLOAD
```
##########
net/devif/devif_iobsend.c:
##########
@@ -53,24 +53,65 @@
****************************************************************************/
void devif_iob_send(FAR struct net_driver_s *dev, FAR struct iob_s *iob,
- unsigned int len, unsigned int offset)
+ unsigned int len, unsigned int offset,
+ unsigned int skip)
{
+ unsigned int copyin;
+ int ret;
+
if (dev == NULL || len == 0 || len >= NETDEV_PKTSIZE(dev))
{
- nerr("devif_iob_send error, %p, send len: %u, pkt len: %u\n",
- dev, len, NETDEV_PKTSIZE(dev));
+ nerr("%s error, %p, send len: %u, pkt len: %u\n",
+ __func__, dev, len, NETDEV_PKTSIZE(dev));
Review Comment:
Is __func__ available for all compilers?
##########
net/devif/devif_poll.c:
##########
@@ -616,12 +616,71 @@ static inline int devif_poll_tcp_connections(FAR struct net_driver_s *dev,
# define devif_poll_tcp_connections(dev, callback) (0)
#endif
+/****************************************************************************
+ * Name: devif_poll_callback
+ *
+ * Description:
+ * This function will help us to gather multiple iob memory slices into a
+ * linear device buffer. if devices with small memory, this function will
+ * trigger a memory copy if net device start transmit the iob slices to
+ * flat buffer
+ *
+ ****************************************************************************/
+
+static int devif_poll_callback(FAR struct net_driver_s *dev)
+{
+ FAR struct iob_s *iob;
+ uint16_t llhdrlen;
+ int bstop = false;
Review Comment:
```suggestion
bool bstop = false;
```
##########
arch/sim/src/sim/up_netdriver.c:
##########
@@ -402,22 +436,21 @@ int netdriver_init(void)
netdriver_txdone_interrupt,
netdriver_rxready_interrupt);
- /* Update the buffer size */
-
- pktsize = dev->d_pktsize ? dev->d_pktsize :
- (MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE);
-
/* Allocate packet buffer */
- pktbuf = kmm_malloc(pktsize);
- if (pktbuf == NULL)
+#ifdef SIM_NETDEV_IOB_OFFLOAD
+ dev->d_buf = NULL;
Review Comment:
```suggestion
dev->d_buf = NULL;
```
##########
net/devif/devif_poll.c:
##########
@@ -616,12 +616,71 @@ static inline int devif_poll_tcp_connections(FAR struct net_driver_s *dev,
# define devif_poll_tcp_connections(dev, callback) (0)
#endif
+/****************************************************************************
+ * Name: devif_poll_callback
+ *
+ * Description:
+ * This function will help us to gather multiple iob memory slices into a
+ * linear device buffer. if devices with small memory, this function will
+ * trigger a memory copy if net device start transmit the iob slices to
+ * flat buffer
+ *
+ ****************************************************************************/
+
+static int devif_poll_callback(FAR struct net_driver_s *dev)
+{
+ FAR struct iob_s *iob;
+ uint16_t llhdrlen;
+ int bstop = false;
+
+ if (dev->d_len > 0 && dev->d_iob != NULL)
+ {
+ llhdrlen = NET_LL_HDRLEN(dev);
+
+ /* Copy iob to flat buffer */
+
+ iob_copyout(dev->d_pollbuf + llhdrlen, dev->d_iob, dev->d_len, 0);
+
+ /* Copy l2 header (arp out) */
+
+ memcpy(dev->d_pollbuf,
+ dev->d_iob->io_data + (CONFIG_NET_LL_GRUARDSIZE - llhdrlen),
+ llhdrlen);
+
+ /* Save iob buffer and restore flat buffer pointer */
+
+ iob = dev->d_iob;
+ dev->d_iob = NULL;
+ dev->d_buf = dev->d_pollbuf;
+
+ /* Call the real device callback */
+
+ bstop = dev->d_pollcallback(dev);
+ if (bstop)
+ {
+ /* Polling stop, release iob buffer */
+
+ iob_free_chain(iob);
+ dev->d_buf = NULL;
+ }
+ else
+ {
+ /* Continue polling, restore iob */
+
+ dev->d_iob = iob;
+ dev->d_buf = (FAR void *)ETHBUF;
Review Comment:
If cast needed there?
##########
net/devif/devif_poll.c:
##########
@@ -639,15 +698,44 @@ static inline int devif_poll_tcp_connections(FAR struct net_driver_s *dev,
* field is set to a value larger than zero. The device driver should then
* send out the packet.
*
+ * This is the iob buffer version of devif_input(),
+ * this function will support send/receive iob vectors directly between
+ * the driver and l3/l4 stack to avoid unnecessary memory copies,
+ * especially on hardware that supports Scatter/gather, which can
+ * greatly improve performance
+ * this function will uses d_iob as packets input which used by some
+ * NICs such as celluler net driver.
+ *
+ * If NIC hardware support Scatter/gather transfer
+ *
+ * tcp_poll()/udp_poll()/pkt_poll()/...(l3/l4)
+ * / \
+ * / \
+ * devif_poll_[l3|l4]_connections() devif_iob_send() (nocopy:udp/icmp/...)
+ * / \ (copy:tcp)
+ * / \
+ * devif_iob_poll("NIC"_txpoll) callback() // "NIC"_txpoll
+ *
+ *
* Assumptions:
* This function is called from the MAC device driver with the network
* locked.
*
****************************************************************************/
-int devif_poll(FAR struct net_driver_s *dev, devif_poll_callback_t callback)
+int devif_iob_poll(FAR struct net_driver_s *dev,
+ devif_poll_callback_t callback)
{
- int bstop = false;
+ int bstop;
Review Comment:
```suggestion
bool bstop;
```
##########
net/icmpv6/icmpv6_input.c:
##########
@@ -87,82 +87,33 @@ static uint16_t icmpv6_datahandler(FAR struct net_driver_s *dev,
unsigned int iplen)
{
FAR struct ipv6_hdr_s *ipv6;
- FAR struct icmpv6_hdr_s *icmpv6;
- FAR struct iob_s *iob;
struct sockaddr_in6 inaddr;
- uint16_t offset;
+ FAR struct iob_s *iob;
uint16_t buflen;
- uint8_t addrsize;
int ret;
- /* Try to allocate on I/O buffer to start the chain without waiting (and
- * throttling as necessary). If we would have to wait, then drop the
- * packet.
- */
-
- iob = iob_tryalloc(true);
- if (iob == NULL)
- {
- nerr("ERROR: Failed to create new I/O buffer chain\n");
- goto drop;
- }
-
/* Put the IPv6 address at the beginning of the read-ahead buffer */
+ iob = dev->d_iob;
ipv6 = IPv6BUF;
inaddr.sin6_family = AF_INET6;
inaddr.sin6_port = 0;
net_ipv6addr_copy(inaddr.sin6_addr.s6_addr16, ipv6->srcipaddr);
- /* Copy the src address info into the I/O buffer chain. We will not wait
- * for an I/O buffer to become available in this context. It there is
- * any failure to allocated, the entire I/O buffer chain will be discarded.
+ /* Copy the src address info into the front of I/O buffer chain which
+ * overwrites the contents of the packet header field.
*/
- addrsize = sizeof(struct sockaddr_in6);
- ret = iob_trycopyin(iob, &addrsize, sizeof(uint8_t), 0, true);
- if (ret < 0)
- {
- /* On a failure, iob_trycopyin return a negated error value but does
- * not free any I/O buffers.
- */
-
- nerr("ERROR: Failed to length to the I/O buffer chain: %d\n", ret);
- goto drop_with_chain;
- }
-
- offset = sizeof(uint8_t);
-
- ret = iob_trycopyin(iob, (FAR const uint8_t *)&inaddr,
- sizeof(struct sockaddr_in6), offset, true);
- if (ret < 0)
- {
- /* On a failure, iob_trycopyin return a negated error value but does
- * not free any I/O buffers.
- */
-
- nerr("ERROR: Failed to source address to the I/O buffer chain: %d\n",
- ret);
- goto drop_with_chain;
- }
-
- offset += sizeof(struct sockaddr_in6);
+ memcpy(iob->io_data, (FAR const uint8_t *)&inaddr,
Review Comment:
Ditto
##########
net/tcp/tcp_send_buffered.c:
##########
@@ -244,6 +244,44 @@ static inline void psock_lost_connection(FAR struct tcp_conn_s *conn,
}
}
+/****************************************************************************
+ * Name: send_tcpip_hdrsize
+ *
+ * Description:
+ * Get the total size of L3 and L4 TCP header
+ *
+ * Input Parameters:
+ * conn The connection structure associated with the socket
+ *
+ * Returned Value:
+ * the total size of L3 and L4 TCP header
+ *
+ ****************************************************************************/
+
+static inline uint16_t send_tcpip_hdrsize(FAR struct tcp_conn_s *conn)
+{
+#if defined(CONFIG_NET_IPv4) && defined(CONFIG_NET_IPv6)
+ if (conn->domain == PF_INET)
+ {
+ /* Select the IPv4 domain */
+
+ return sizeof(struct ipv4_hdr_s) + sizeof(struct tcp_hdr_s);
+ }
+ else /* if (domain == PF_INET6) */
+ {
+ /* Select the IPv6 domain */
+
+ return sizeof(struct ipv6_hdr_s) + sizeof(struct tcp_hdr_s);
+ }
+#elif defined(CONFIG_NET_IPv4)
+ ((void)conn);
Review Comment:
Should we use `UNUSED` macro instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org