lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <208fb61b-4740-46bf-8c70-29ab59cbb965@lunn.ch>
Date: Thu, 7 Mar 2024 18:08:52 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, saeedm@...dia.com,
	anthony.l.nguyen@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, corbet@....net,
	linux-doc@...r.kernel.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	devicetree@...r.kernel.org, horatiu.vultur@...rochip.com,
	ruanjinjie@...wei.com, steen.hegelund@...rochip.com,
	vladimir.oltean@....com, UNGLinuxDriver@...rochip.com,
	Thorsten.Kummermehr@...rochip.com, Pier.Beruto@...emi.com,
	Selvamani.Rajagopal@...emi.com, Nicolas.Ferre@...rochip.com,
	benjamin.bigler@...nformulastudent.ch
Subject: Re: [PATCH net-next v3 08/12] net: ethernet: oa_tc6: implement
 transmit path to transfer tx ethernet frames

> @@ -55,6 +77,14 @@
>  						(OA_TC6_CTRL_MAX_REGISTERS *\
>  						OA_TC6_CTRL_REG_VALUE_SIZE) +\
>  						OA_TC6_CTRL_IGNORED_SIZE)
> +#define OA_TC6_CHUNK_PAYLOAD_SIZE		64
> +#define OA_TC6_DATA_HEADER_SIZE			4
> +#define OA_TC6_CHUNK_SIZE			(OA_TC6_DATA_HEADER_SIZE +\
> +						OA_TC6_CHUNK_PAYLOAD_SIZE)
> +#define OA_TC6_TX_SKB_QUEUE_SIZE		100

So you keep up to 100 packets in a queue. If use assume typical MTU
size packets, that is 1,238,400 bits. At 10Mbps, that is 120ms of
traffic. That is quite a lot of latency when a high priority packet is
added to the tail of the queue and needs to wait for all the other
packets to be sent first.

Chunks are 64 bytes. So in practice, you only ever need two
packets. You need to be able to fill a chunk with the final part of
one packet, and the beginning of the next. So i would try using a much
smaller queue size. That will allow Linux queue disciplines to give
you the high priority packets first which you send with low latency.

> +static void oa_tc6_add_tx_skb_to_spi_buf(struct oa_tc6 *tc6)
> +{
> +	enum oa_tc6_data_start_valid_info start_valid = OA_TC6_DATA_START_INVALID;
> +	enum oa_tc6_data_end_valid_info end_valid = OA_TC6_DATA_END_INVALID;
> +	__be32 *tx_buf = tc6->spi_data_tx_buf + tc6->spi_data_tx_buf_offset;
> +	u16 remaining_length = tc6->tx_skb->len - tc6->tx_skb_offset;
> +	u8 *tx_skb_data = tc6->tx_skb->data + tc6->tx_skb_offset;
> +	u8 end_byte_offset = 0;
> +	u16 length_to_copy;
> +
> +	/* Set start valid if the current tx chunk contains the start of the tx
> +	 * ethernet frame.
> +	 */
> +	if (!tc6->tx_skb_offset)
> +		start_valid = OA_TC6_DATA_START_VALID;
> +
> +	/* If the remaining tx skb length is more than the chunk payload size of
> +	 * 64 bytes then copy only 64 bytes and leave the ongoing tx skb for
> +	 * next tx chunk.
> +	 */
> +	length_to_copy = min_t(u16, remaining_length, OA_TC6_CHUNK_PAYLOAD_SIZE);
> +
> +	/* Copy the tx skb data to the tx chunk payload buffer */
> +	memcpy(tx_buf + 1, tx_skb_data, length_to_copy);
> +	tc6->tx_skb_offset += length_to_copy;

You probably need a call to skb_linearize() somewhere. You assume the
packet data is contiguous. It can in fact be split into multiple
segments. skb_linearize() will convert it to a single buffer.

> +static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)
> +{
> +	int ret;
> +
> +	while (true) {
> +		u16 spi_length = 0;
> +
> +		tc6->spi_data_tx_buf_offset = 0;
> +
> +		if (tc6->tx_skb || !skb_queue_empty(&tc6->tx_skb_q))
> +			spi_length = oa_tc6_prepare_spi_tx_buf_for_tx_skbs(tc6);
> +
> +		if (spi_length == 0)
> +			break;
> +
> +		ret = oa_tc6_spi_transfer(tc6, OA_TC6_DATA_HEADER, spi_length);
> +		if (ret) {
> +			netdev_err(tc6->netdev,
> +				   "SPI data transfer failed. Restart the system: %d\n",
> +				   ret);

What does Restart the system mean?

> +static int oa_tc6_spi_thread_handler(void *data)
> +{
> +	struct oa_tc6 *tc6 = data;
> +	int ret;
> +
> +	while (likely(!kthread_should_stop())) {
> +		/* This kthread will be waken up if there is a tx skb */
> +		wait_event_interruptible(tc6->spi_wq,
> +					 !skb_queue_empty(&tc6->tx_skb_q) ||
> +					 kthread_should_stop());
> +		ret = oa_tc6_try_spi_transfer(tc6);

Shouldn't you check why you have been woken up? It seems more logical
to test here for kthread_should_stop() rather than have
oa_tc6_try_spi_transfer() handle there is not actually a packet to be
sent.

	Andrew

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ