* [PATCH net-next v2] net/tls: Add support for async encryption of records for performance
From: Vakul Garg @ 2018-09-21 4:16 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, doronrk, Vakul Garg
In current implementation, tls records are encrypted & transmitted
serially. Till the time the previously submitted user data is encrypted,
the implementation waits and on finish starts transmitting the record.
This approach of encrypt-one record at a time is inefficient when
asynchronous crypto accelerators are used. For each record, there are
overheads of interrupts, driver softIRQ scheduling etc. Also the crypto
accelerator sits idle most of time while an encrypted record's pages are
handed over to tcp stack for transmission.
This patch enables encryption of multiple records in parallel when an
async capable crypto accelerator is present in system. This is achieved
by allowing the user space application to send more data using sendmsg()
even while previously issued data is being processed by crypto
accelerator. This requires returning the control back to user space
application after submitting encryption request to accelerator. This
also means that zero-copy mode of encryption cannot be used with async
accelerator as we must be done with user space application buffer before
returning from sendmsg().
There can be multiple records in flight to/from the accelerator. Each of
the record is represented by 'struct tls_rec'. This is used to store the
memory pages for the record.
After the records are encrypted, they are added in a linked list called
tx_ready_list which contains encrypted tls records sorted as per tls
sequence number. The records from tx_ready_list are transmitted using a
newly introduced function called tls_tx_records(). The tx_ready_list is
polled for any record ready to be transmitted in sendmsg(), sendpage()
after initiating encryption of new tls records. This achieves parallel
encryption and transmission of records when async accelerator is
present.
There could be situation when crypto accelerator completes encryption
later than polling of tx_ready_list by sendmsg()/sendpage(). Therefore
we need a deferred work context to be able to transmit records from
tx_ready_list. The deferred work context gets scheduled if applications
are not sending much data through the socket. If the applications issue
sendmsg()/sendpage() in quick succession, then the scheduling of
tx_work_handler gets cancelled as the tx_ready_list would be polled from
application's context itself. This saves scheduling overhead of deferred
work.
The patch also brings some side benefit. We are able to get rid of the
concept of CLOSED record. This is because the records once closed are
either encrypted and then placed into tx_ready_list or if encryption
fails, the socket error is set. This simplifies the kernel tls
sendpath. However since tls_device.c is still using macros, accessory
functions for CLOSED records have been retained.
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
Changes since v1: Addressed Dave Miller's comments.
- Removed an extra space between 'inline' and 'bool' in
'is_tx_ready' declaration.
- Changed order of variable declarations at several places in
order to following 'longest to shortest' line suggestion.
include/net/tls.h | 70 +++++--
net/tls/tls_main.c | 54 ++---
net/tls/tls_sw.c | 586 ++++++++++++++++++++++++++++++++++++++++-------------
3 files changed, 522 insertions(+), 188 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index 9f3c4ea9ad6f..3aa73e2d8823 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -41,7 +41,7 @@
#include <linux/tcp.h>
#include <net/tcp.h>
#include <net/strparser.h>
-
+#include <crypto/aead.h>
#include <uapi/linux/tls.h>
@@ -93,24 +93,47 @@ enum {
TLS_NUM_CONFIG,
};
-struct tls_sw_context_tx {
- struct crypto_aead *aead_send;
- struct crypto_wait async_wait;
-
- char aad_space[TLS_AAD_SPACE_SIZE];
-
- unsigned int sg_plaintext_size;
- int sg_plaintext_num_elem;
+/* TLS records are maintained in 'struct tls_rec'. It stores the memory pages
+ * allocated or mapped for each TLS record. After encryption, the records are
+ * stores in a linked list.
+ */
+struct tls_rec {
+ struct list_head list;
+ int tx_flags;
struct scatterlist sg_plaintext_data[MAX_SKB_FRAGS];
-
- unsigned int sg_encrypted_size;
- int sg_encrypted_num_elem;
struct scatterlist sg_encrypted_data[MAX_SKB_FRAGS];
/* AAD | sg_plaintext_data | sg_tag */
struct scatterlist sg_aead_in[2];
/* AAD | sg_encrypted_data (data contain overhead for hdr&iv&tag) */
struct scatterlist sg_aead_out[2];
+
+ unsigned int sg_plaintext_size;
+ unsigned int sg_encrypted_size;
+ int sg_plaintext_num_elem;
+ int sg_encrypted_num_elem;
+
+ char aad_space[TLS_AAD_SPACE_SIZE];
+ struct aead_request aead_req;
+ u8 aead_req_ctx[];
+};
+
+struct tx_work {
+ struct delayed_work work;
+ struct sock *sk;
+};
+
+struct tls_sw_context_tx {
+ struct crypto_aead *aead_send;
+ struct crypto_wait async_wait;
+ struct tx_work tx_work;
+ struct tls_rec *open_rec;
+ struct list_head tx_ready_list;
+ atomic_t encrypt_pending;
+ int async_notify;
+
+#define BIT_TX_SCHEDULED 0
+ unsigned long tx_bitmask;
};
struct tls_sw_context_rx {
@@ -197,6 +220,8 @@ struct tls_context {
struct scatterlist *partially_sent_record;
u16 partially_sent_offset;
+ u64 tx_seq_number; /* Next TLS seqnum to be transmitted */
+
unsigned long flags;
bool in_tcp_sendpages;
@@ -261,6 +286,7 @@ int tls_device_sendpage(struct sock *sk, struct page *page,
void tls_device_sk_destruct(struct sock *sk);
void tls_device_init(void);
void tls_device_cleanup(void);
+int tls_tx_records(struct sock *sk, int flags);
struct tls_record_info *tls_get_record(struct tls_offload_context_tx *context,
u32 seq, u64 *p_record_sn);
@@ -279,6 +305,9 @@ void tls_sk_destruct(struct sock *sk, struct tls_context *ctx);
int tls_push_sg(struct sock *sk, struct tls_context *ctx,
struct scatterlist *sg, u16 first_offset,
int flags);
+int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
+ int flags);
+
int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
int flags, long *timeo);
@@ -312,6 +341,23 @@ static inline bool tls_is_pending_open_record(struct tls_context *tls_ctx)
return tls_ctx->pending_open_record_frags;
}
+static inline bool is_tx_ready(struct tls_context *tls_ctx,
+ struct tls_sw_context_tx *ctx)
+{
+ struct tls_rec *rec;
+ u64 seq;
+
+ rec = list_first_entry(&ctx->tx_ready_list, struct tls_rec, list);
+ if (!rec)
+ return false;
+
+ seq = be64_to_cpup((const __be64 *)&rec->aad_space);
+ if (seq == tls_ctx->tx_seq_number)
+ return true;
+ else
+ return false;
+}
+
struct sk_buff *
tls_validate_xmit_skb(struct sock *sk, struct net_device *dev,
struct sk_buff *skb);
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 523622dc74f8..06094de7a3d9 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -141,7 +141,6 @@ int tls_push_sg(struct sock *sk,
size = sg->length;
}
- clear_bit(TLS_PENDING_CLOSED_RECORD, &ctx->flags);
ctx->in_tcp_sendpages = false;
ctx->sk_write_space(sk);
@@ -193,15 +192,12 @@ int tls_proccess_cmsg(struct sock *sk, struct msghdr *msg,
return rc;
}
-int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
- int flags, long *timeo)
+int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
+ int flags)
{
struct scatterlist *sg;
u16 offset;
- if (!tls_is_partially_sent_record(ctx))
- return ctx->push_pending_record(sk, flags);
-
sg = ctx->partially_sent_record;
offset = ctx->partially_sent_offset;
@@ -209,9 +205,23 @@ int tls_push_pending_closed_record(struct sock *sk, struct tls_context *ctx,
return tls_push_sg(sk, ctx, sg, offset, flags);
}
+int tls_push_pending_closed_record(struct sock *sk,
+ struct tls_context *tls_ctx,
+ int flags, long *timeo)
+{
+ struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+
+ if (tls_is_partially_sent_record(tls_ctx) ||
+ !list_empty(&ctx->tx_ready_list))
+ return tls_tx_records(sk, flags);
+ else
+ return tls_ctx->push_pending_record(sk, flags);
+}
+
static void tls_write_space(struct sock *sk)
{
struct tls_context *ctx = tls_get_ctx(sk);
+ struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
/* If in_tcp_sendpages call lower protocol write space handler
* to ensure we wake up any waiting operations there. For example
@@ -222,20 +232,11 @@ static void tls_write_space(struct sock *sk)
return;
}
- if (!sk->sk_write_pending && tls_is_pending_closed_record(ctx)) {
- gfp_t sk_allocation = sk->sk_allocation;
- int rc;
- long timeo = 0;
-
- sk->sk_allocation = GFP_ATOMIC;
- rc = tls_push_pending_closed_record(sk, ctx,
- MSG_DONTWAIT |
- MSG_NOSIGNAL,
- &timeo);
- sk->sk_allocation = sk_allocation;
-
- if (rc < 0)
- return;
+ /* Schedule the transmission if tx list is ready */
+ if (is_tx_ready(ctx, tx_ctx) && !sk->sk_write_pending) {
+ /* Schedule the transmission */
+ if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx->tx_bitmask))
+ schedule_delayed_work(&tx_ctx->tx_work.work, 0);
}
ctx->sk_write_space(sk);
@@ -270,19 +271,6 @@ static void tls_sk_proto_close(struct sock *sk, long timeout)
if (!tls_complete_pending_work(sk, ctx, 0, &timeo))
tls_handle_open_record(sk, 0);
- if (ctx->partially_sent_record) {
- struct scatterlist *sg = ctx->partially_sent_record;
-
- while (1) {
- put_page(sg_page(sg));
- sk_mem_uncharge(sk, sg->length);
-
- if (sg_is_last(sg))
- break;
- sg++;
- }
- }
-
/* We need these for tls_sw_fallback handling of other packets */
if (ctx->tx_conf == TLS_SW) {
kfree(ctx->tx.rec_seq);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 5ff51bac8b46..bcb24c498b84 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -246,18 +246,19 @@ static void trim_both_sgl(struct sock *sk, int target_size)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec = ctx->open_rec;
- trim_sg(sk, ctx->sg_plaintext_data,
- &ctx->sg_plaintext_num_elem,
- &ctx->sg_plaintext_size,
+ trim_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size,
target_size);
if (target_size > 0)
target_size += tls_ctx->tx.overhead_size;
- trim_sg(sk, ctx->sg_encrypted_data,
- &ctx->sg_encrypted_num_elem,
- &ctx->sg_encrypted_size,
+ trim_sg(sk, rec->sg_encrypted_data,
+ &rec->sg_encrypted_num_elem,
+ &rec->sg_encrypted_size,
target_size);
}
@@ -265,15 +266,16 @@ static int alloc_encrypted_sg(struct sock *sk, int len)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec = ctx->open_rec;
int rc = 0;
rc = sk_alloc_sg(sk, len,
- ctx->sg_encrypted_data, 0,
- &ctx->sg_encrypted_num_elem,
- &ctx->sg_encrypted_size, 0);
+ rec->sg_encrypted_data, 0,
+ &rec->sg_encrypted_num_elem,
+ &rec->sg_encrypted_size, 0);
if (rc == -ENOSPC)
- ctx->sg_encrypted_num_elem = ARRAY_SIZE(ctx->sg_encrypted_data);
+ rec->sg_encrypted_num_elem = ARRAY_SIZE(rec->sg_encrypted_data);
return rc;
}
@@ -282,14 +284,15 @@ static int alloc_plaintext_sg(struct sock *sk, int len)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec = ctx->open_rec;
int rc = 0;
- rc = sk_alloc_sg(sk, len, ctx->sg_plaintext_data, 0,
- &ctx->sg_plaintext_num_elem, &ctx->sg_plaintext_size,
+ rc = sk_alloc_sg(sk, len, rec->sg_plaintext_data, 0,
+ &rec->sg_plaintext_num_elem, &rec->sg_plaintext_size,
tls_ctx->pending_open_record_frags);
if (rc == -ENOSPC)
- ctx->sg_plaintext_num_elem = ARRAY_SIZE(ctx->sg_plaintext_data);
+ rec->sg_plaintext_num_elem = ARRAY_SIZE(rec->sg_plaintext_data);
return rc;
}
@@ -311,37 +314,192 @@ static void tls_free_both_sg(struct sock *sk)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec = ctx->open_rec;
- free_sg(sk, ctx->sg_encrypted_data, &ctx->sg_encrypted_num_elem,
- &ctx->sg_encrypted_size);
+ /* Return if there is no open record */
+ if (!rec)
+ return;
+
+ free_sg(sk, rec->sg_encrypted_data,
+ &rec->sg_encrypted_num_elem,
+ &rec->sg_encrypted_size);
+
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size);
+}
+
+static bool append_tx_ready_list(struct tls_context *tls_ctx,
+ struct tls_sw_context_tx *ctx,
+ struct tls_rec *enc_rec)
+{
+ u64 new_seq = be64_to_cpup((const __be64 *)&enc_rec->aad_space);
+ struct list_head *pos;
+
+ /* Need to insert encrypted record in tx_ready_list sorted
+ * as per sequence number. Traverse linked list from tail.
+ */
+ list_for_each_prev(pos, &ctx->tx_ready_list) {
+ struct tls_rec *rec = (struct tls_rec *)pos;
+ u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
+
+ if (new_seq > seq)
+ break;
+ }
+
+ list_add((struct list_head *)&enc_rec->list, pos);
+
+ return is_tx_ready(tls_ctx, ctx);
+}
+
+int tls_tx_records(struct sock *sk, int flags)
+{
+ struct tls_context *tls_ctx = tls_get_ctx(sk);
+ struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec, *tmp;
+ int tx_flags, rc = 0;
+
+ if (tls_is_partially_sent_record(tls_ctx)) {
+ rec = list_first_entry(&ctx->tx_ready_list,
+ struct tls_rec, list);
+
+ if (flags == -1)
+ tx_flags = rec->tx_flags;
+ else
+ tx_flags = flags;
+
+ rc = tls_push_partial_record(sk, tls_ctx, tx_flags);
+ if (rc)
+ goto tx_err;
+
+ /* Full record has been transmitted.
+ * Remove the head of tx_ready_list
+ */
+ tls_ctx->tx_seq_number++;
+ list_del(&rec->list);
+ kfree(rec);
+ }
+
+ /* Tx all ready records which have expected sequence number */
+ list_for_each_entry_safe(rec, tmp, &ctx->tx_ready_list, list) {
+ u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
+
+ if (seq == tls_ctx->tx_seq_number) {
+ if (flags == -1)
+ tx_flags = rec->tx_flags;
+ else
+ tx_flags = flags;
+
+ rc = tls_push_sg(sk, tls_ctx,
+ &rec->sg_encrypted_data[0],
+ 0, tx_flags);
+ if (rc)
+ goto tx_err;
+
+ tls_ctx->tx_seq_number++;
+ list_del(&rec->list);
+ kfree(rec);
+ } else {
+ break;
+ }
+ }
+
+tx_err:
+ if (rc < 0 && rc != -EAGAIN)
+ tls_err_abort(sk, EBADMSG);
+
+ return rc;
+}
+
+static void tls_encrypt_done(struct crypto_async_request *req, int err)
+{
+ struct aead_request *aead_req = (struct aead_request *)req;
+ struct sock *sk = req->data;
+ struct tls_context *tls_ctx = tls_get_ctx(sk);
+ struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec;
+ bool ready = false;
+ int pending;
+
+ rec = container_of(aead_req, struct tls_rec, aead_req);
+
+ rec->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
+ rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
+
+ free_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem, &rec->sg_plaintext_size);
+
+ /* Free the record if error is previously set on socket */
+ if (err || sk->sk_err) {
+ free_sg(sk, rec->sg_encrypted_data,
+ &rec->sg_encrypted_num_elem, &rec->sg_encrypted_size);
- free_sg(sk, ctx->sg_plaintext_data, &ctx->sg_plaintext_num_elem,
- &ctx->sg_plaintext_size);
+ kfree(rec);
+ rec = NULL;
+
+ /* If err is already set on socket, return the same code */
+ if (sk->sk_err) {
+ ctx->async_wait.err = sk->sk_err;
+ } else {
+ ctx->async_wait.err = err;
+ tls_err_abort(sk, err);
+ }
+ }
+
+ /* Append the record in tx queue */
+ if (rec)
+ ready = append_tx_ready_list(tls_ctx, ctx, rec);
+
+ pending = atomic_dec_return(&ctx->encrypt_pending);
+
+ if (!pending && READ_ONCE(ctx->async_notify))
+ complete(&ctx->async_wait.completion);
+
+ if (!ready)
+ return;
+
+ /* Schedule the transmission */
+ if (!test_and_set_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
+ schedule_delayed_work(&ctx->tx_work.work, 1);
}
-static int tls_do_encryption(struct tls_context *tls_ctx,
+static int tls_do_encryption(struct sock *sk,
+ struct tls_context *tls_ctx,
struct tls_sw_context_tx *ctx,
struct aead_request *aead_req,
size_t data_len)
{
+ struct tls_rec *rec = ctx->open_rec;
int rc;
- ctx->sg_encrypted_data[0].offset += tls_ctx->tx.prepend_size;
- ctx->sg_encrypted_data[0].length -= tls_ctx->tx.prepend_size;
+ rec->sg_encrypted_data[0].offset += tls_ctx->tx.prepend_size;
+ rec->sg_encrypted_data[0].length -= tls_ctx->tx.prepend_size;
aead_request_set_tfm(aead_req, ctx->aead_send);
aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
- aead_request_set_crypt(aead_req, ctx->sg_aead_in, ctx->sg_aead_out,
+ aead_request_set_crypt(aead_req, rec->sg_aead_in,
+ rec->sg_aead_out,
data_len, tls_ctx->tx.iv);
aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_BACKLOG,
- crypto_req_done, &ctx->async_wait);
+ tls_encrypt_done, sk);
+
+ atomic_inc(&ctx->encrypt_pending);
- rc = crypto_wait_req(crypto_aead_encrypt(aead_req), &ctx->async_wait);
+ rc = crypto_aead_encrypt(aead_req);
+ if (!rc || rc != -EINPROGRESS) {
+ atomic_dec(&ctx->encrypt_pending);
+ rec->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
+ rec->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
+ }
- ctx->sg_encrypted_data[0].offset -= tls_ctx->tx.prepend_size;
- ctx->sg_encrypted_data[0].length += tls_ctx->tx.prepend_size;
+ /* Case of encryption failure */
+ if (rc && rc != -EINPROGRESS)
+ return rc;
+ /* Unhook the record from context if encryption is not failure */
+ ctx->open_rec = NULL;
+ tls_advance_record_sn(sk, &tls_ctx->tx);
return rc;
}
@@ -350,53 +508,49 @@ static int tls_push_record(struct sock *sk, int flags,
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec = ctx->open_rec;
struct aead_request *req;
int rc;
- req = aead_request_alloc(ctx->aead_send, sk->sk_allocation);
- if (!req)
- return -ENOMEM;
+ if (!rec)
+ return 0;
- sg_mark_end(ctx->sg_plaintext_data + ctx->sg_plaintext_num_elem - 1);
- sg_mark_end(ctx->sg_encrypted_data + ctx->sg_encrypted_num_elem - 1);
+ rec->tx_flags = flags;
+ req = &rec->aead_req;
- tls_make_aad(ctx->aad_space, ctx->sg_plaintext_size,
+ sg_mark_end(rec->sg_plaintext_data + rec->sg_plaintext_num_elem - 1);
+ sg_mark_end(rec->sg_encrypted_data + rec->sg_encrypted_num_elem - 1);
+
+ tls_make_aad(rec->aad_space, rec->sg_plaintext_size,
tls_ctx->tx.rec_seq, tls_ctx->tx.rec_seq_size,
record_type);
tls_fill_prepend(tls_ctx,
- page_address(sg_page(&ctx->sg_encrypted_data[0])) +
- ctx->sg_encrypted_data[0].offset,
- ctx->sg_plaintext_size, record_type);
+ page_address(sg_page(&rec->sg_encrypted_data[0])) +
+ rec->sg_encrypted_data[0].offset,
+ rec->sg_plaintext_size, record_type);
tls_ctx->pending_open_record_frags = 0;
- set_bit(TLS_PENDING_CLOSED_RECORD, &tls_ctx->flags);
-
- rc = tls_do_encryption(tls_ctx, ctx, req, ctx->sg_plaintext_size);
- if (rc < 0) {
- /* If we are called from write_space and
- * we fail, we need to set this SOCK_NOSPACE
- * to trigger another write_space in the future.
- */
- set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
- goto out_req;
- }
- free_sg(sk, ctx->sg_plaintext_data, &ctx->sg_plaintext_num_elem,
- &ctx->sg_plaintext_size);
+ rc = tls_do_encryption(sk, tls_ctx, ctx, req, rec->sg_plaintext_size);
+ if (rc == -EINPROGRESS)
+ return -EINPROGRESS;
- ctx->sg_encrypted_num_elem = 0;
- ctx->sg_encrypted_size = 0;
+ free_sg(sk, rec->sg_plaintext_data, &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size);
- /* Only pass through MSG_DONTWAIT and MSG_NOSIGNAL flags */
- rc = tls_push_sg(sk, tls_ctx, ctx->sg_encrypted_data, 0, flags);
- if (rc < 0 && rc != -EAGAIN)
+ if (rc < 0) {
tls_err_abort(sk, EBADMSG);
+ return rc;
+ }
- tls_advance_record_sn(sk, &tls_ctx->tx);
-out_req:
- aead_request_free(req);
- return rc;
+ /* Put the record in tx_ready_list and start tx if permitted.
+ * This happens only when encryption is not asynchronous.
+ */
+ if (append_tx_ready_list(tls_ctx, ctx, rec))
+ return tls_tx_records(sk, flags);
+
+ return 0;
}
static int tls_sw_push_pending_record(struct sock *sk, int flags)
@@ -473,11 +627,12 @@ static int memcopy_from_iter(struct sock *sk, struct iov_iter *from,
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
- struct scatterlist *sg = ctx->sg_plaintext_data;
+ struct tls_rec *rec = ctx->open_rec;
+ struct scatterlist *sg = rec->sg_plaintext_data;
int copy, i, rc = 0;
for (i = tls_ctx->pending_open_record_frags;
- i < ctx->sg_plaintext_num_elem; ++i) {
+ i < rec->sg_plaintext_num_elem; ++i) {
copy = sg[i].length;
if (copy_from_iter(
page_address(sg_page(&sg[i])) + sg[i].offset,
@@ -497,34 +652,85 @@ static int memcopy_from_iter(struct sock *sk, struct iov_iter *from,
return rc;
}
-int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
+struct tls_rec *get_rec(struct sock *sk)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
- int ret;
- int required_size;
+ struct tls_rec *rec;
+ int mem_size;
+
+ /* Return if we already have an open record */
+ if (ctx->open_rec)
+ return ctx->open_rec;
+
+ mem_size = sizeof(struct tls_rec) + crypto_aead_reqsize(ctx->aead_send);
+
+ rec = kzalloc(mem_size, sk->sk_allocation);
+ if (!rec)
+ return NULL;
+
+ sg_init_table(&rec->sg_plaintext_data[0],
+ ARRAY_SIZE(rec->sg_plaintext_data));
+ sg_init_table(&rec->sg_encrypted_data[0],
+ ARRAY_SIZE(rec->sg_encrypted_data));
+
+ sg_init_table(rec->sg_aead_in, 2);
+ sg_set_buf(&rec->sg_aead_in[0], rec->aad_space,
+ sizeof(rec->aad_space));
+ sg_unmark_end(&rec->sg_aead_in[1]);
+ sg_chain(rec->sg_aead_in, 2, rec->sg_plaintext_data);
+
+ sg_init_table(rec->sg_aead_out, 2);
+ sg_set_buf(&rec->sg_aead_out[0], rec->aad_space,
+ sizeof(rec->aad_space));
+ sg_unmark_end(&rec->sg_aead_out[1]);
+ sg_chain(rec->sg_aead_out, 2, rec->sg_encrypted_data);
+
+ ctx->open_rec = rec;
+
+ return rec;
+}
+
+int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
+{
long timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
+ struct tls_context *tls_ctx = tls_get_ctx(sk);
+ struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
+ bool async_capable = tfm->__crt_alg->cra_flags & CRYPTO_ALG_ASYNC;
+ unsigned char record_type = TLS_RECORD_TYPE_DATA;
+ bool is_kvec = msg->msg_iter.type & ITER_KVEC;
bool eor = !(msg->msg_flags & MSG_MORE);
size_t try_to_copy, copied = 0;
- unsigned char record_type = TLS_RECORD_TYPE_DATA;
- int record_room;
+ struct tls_rec *rec;
+ int required_size;
+ int num_async = 0;
bool full_record;
+ int record_room;
+ int num_zc = 0;
int orig_size;
- bool is_kvec = msg->msg_iter.type & ITER_KVEC;
+ int ret;
if (msg->msg_flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL))
return -ENOTSUPP;
lock_sock(sk);
- ret = tls_complete_pending_work(sk, tls_ctx, msg->msg_flags, &timeo);
- if (ret)
- goto send_end;
+ /* Wait till there is any pending write on socket */
+ if (unlikely(sk->sk_write_pending)) {
+ ret = wait_on_pending_writer(sk, &timeo);
+ if (unlikely(ret))
+ goto send_end;
+ }
if (unlikely(msg->msg_controllen)) {
ret = tls_proccess_cmsg(sk, msg, &record_type);
- if (ret)
- goto send_end;
+ if (ret) {
+ if (ret == -EINPROGRESS)
+ num_async++;
+ else if (ret != -EAGAIN)
+ goto send_end;
+ }
}
while (msg_data_left(msg)) {
@@ -533,20 +739,27 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
goto send_end;
}
- orig_size = ctx->sg_plaintext_size;
+ rec = get_rec(sk);
+ if (!rec) {
+ ret = -ENOMEM;
+ goto send_end;
+ }
+
+ orig_size = rec->sg_plaintext_size;
full_record = false;
try_to_copy = msg_data_left(msg);
- record_room = TLS_MAX_PAYLOAD_SIZE - ctx->sg_plaintext_size;
+ record_room = TLS_MAX_PAYLOAD_SIZE - rec->sg_plaintext_size;
if (try_to_copy >= record_room) {
try_to_copy = record_room;
full_record = true;
}
- required_size = ctx->sg_plaintext_size + try_to_copy +
+ required_size = rec->sg_plaintext_size + try_to_copy +
tls_ctx->tx.overhead_size;
if (!sk_stream_memory_free(sk))
goto wait_for_sndbuf;
+
alloc_encrypted:
ret = alloc_encrypted_sg(sk, required_size);
if (ret) {
@@ -557,33 +770,39 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
* actually allocated. The difference is due
* to max sg elements limit
*/
- try_to_copy -= required_size - ctx->sg_encrypted_size;
+ try_to_copy -= required_size - rec->sg_encrypted_size;
full_record = true;
}
- if (!is_kvec && (full_record || eor)) {
+
+ if (!is_kvec && (full_record || eor) && !async_capable) {
ret = zerocopy_from_iter(sk, &msg->msg_iter,
- try_to_copy, &ctx->sg_plaintext_num_elem,
- &ctx->sg_plaintext_size,
- ctx->sg_plaintext_data,
- ARRAY_SIZE(ctx->sg_plaintext_data),
+ try_to_copy, &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size,
+ rec->sg_plaintext_data,
+ ARRAY_SIZE(rec->sg_plaintext_data),
true);
if (ret)
goto fallback_to_reg_send;
+ num_zc++;
copied += try_to_copy;
ret = tls_push_record(sk, msg->msg_flags, record_type);
- if (ret)
- goto send_end;
+ if (ret) {
+ if (ret == -EINPROGRESS)
+ num_async++;
+ else if (ret != -EAGAIN)
+ goto send_end;
+ }
continue;
fallback_to_reg_send:
- trim_sg(sk, ctx->sg_plaintext_data,
- &ctx->sg_plaintext_num_elem,
- &ctx->sg_plaintext_size,
+ trim_sg(sk, rec->sg_plaintext_data,
+ &rec->sg_plaintext_num_elem,
+ &rec->sg_plaintext_size,
orig_size);
}
- required_size = ctx->sg_plaintext_size + try_to_copy;
+ required_size = rec->sg_plaintext_size + try_to_copy;
alloc_plaintext:
ret = alloc_plaintext_sg(sk, required_size);
if (ret) {
@@ -594,13 +813,13 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
* actually allocated. The difference is due
* to max sg elements limit
*/
- try_to_copy -= required_size - ctx->sg_plaintext_size;
+ try_to_copy -= required_size - rec->sg_plaintext_size;
full_record = true;
- trim_sg(sk, ctx->sg_encrypted_data,
- &ctx->sg_encrypted_num_elem,
- &ctx->sg_encrypted_size,
- ctx->sg_plaintext_size +
+ trim_sg(sk, rec->sg_encrypted_data,
+ &rec->sg_encrypted_num_elem,
+ &rec->sg_encrypted_size,
+ rec->sg_plaintext_size +
tls_ctx->tx.overhead_size);
}
@@ -610,13 +829,12 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
copied += try_to_copy;
if (full_record || eor) {
-push_record:
ret = tls_push_record(sk, msg->msg_flags, record_type);
if (ret) {
- if (ret == -ENOMEM)
- goto wait_for_memory;
-
- goto send_end;
+ if (ret == -EINPROGRESS)
+ num_async++;
+ else if (ret != -EAGAIN)
+ goto send_end;
}
}
@@ -632,15 +850,37 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
goto send_end;
}
- if (tls_is_pending_closed_record(tls_ctx))
- goto push_record;
-
- if (ctx->sg_encrypted_size < required_size)
+ if (rec->sg_encrypted_size < required_size)
goto alloc_encrypted;
goto alloc_plaintext;
}
+ if (!num_async) {
+ goto send_end;
+ } else if (num_zc) {
+ /* Wait for pending encryptions to get completed */
+ smp_store_mb(ctx->async_notify, true);
+
+ if (atomic_read(&ctx->encrypt_pending))
+ crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
+ else
+ reinit_completion(&ctx->async_wait.completion);
+
+ WRITE_ONCE(ctx->async_notify, false);
+
+ if (ctx->async_wait.err) {
+ ret = ctx->async_wait.err;
+ copied = 0;
+ }
+ }
+
+ /* Transmit if any encryptions have completed */
+ if (test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask)) {
+ cancel_delayed_work(&ctx->tx_work.work);
+ tls_tx_records(sk, msg->msg_flags);
+ }
+
send_end:
ret = sk_stream_error(sk, msg->msg_flags, ret);
@@ -651,16 +891,18 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
int tls_sw_sendpage(struct sock *sk, struct page *page,
int offset, size_t size, int flags)
{
+ long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
- int ret;
- long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT);
- bool eor;
- size_t orig_size = size;
unsigned char record_type = TLS_RECORD_TYPE_DATA;
+ size_t orig_size = size;
struct scatterlist *sg;
+ struct tls_rec *rec;
+ int num_async = 0;
bool full_record;
int record_room;
+ bool eor;
+ int ret;
if (flags & ~(MSG_MORE | MSG_DONTWAIT | MSG_NOSIGNAL |
MSG_SENDPAGE_NOTLAST))
@@ -673,9 +915,12 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
- ret = tls_complete_pending_work(sk, tls_ctx, flags, &timeo);
- if (ret)
- goto sendpage_end;
+ /* Wait till there is any pending write on socket */
+ if (unlikely(sk->sk_write_pending)) {
+ ret = wait_on_pending_writer(sk, &timeo);
+ if (unlikely(ret))
+ goto sendpage_end;
+ }
/* Call the sk_stream functions to manage the sndbuf mem. */
while (size > 0) {
@@ -686,14 +931,20 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
goto sendpage_end;
}
+ rec = get_rec(sk);
+ if (!rec) {
+ ret = -ENOMEM;
+ goto sendpage_end;
+ }
+
full_record = false;
- record_room = TLS_MAX_PAYLOAD_SIZE - ctx->sg_plaintext_size;
+ record_room = TLS_MAX_PAYLOAD_SIZE - rec->sg_plaintext_size;
copy = size;
if (copy >= record_room) {
copy = record_room;
full_record = true;
}
- required_size = ctx->sg_plaintext_size + copy +
+ required_size = rec->sg_plaintext_size + copy +
tls_ctx->tx.overhead_size;
if (!sk_stream_memory_free(sk))
@@ -708,33 +959,32 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
* actually allocated. The difference is due
* to max sg elements limit
*/
- copy -= required_size - ctx->sg_plaintext_size;
+ copy -= required_size - rec->sg_plaintext_size;
full_record = true;
}
get_page(page);
- sg = ctx->sg_plaintext_data + ctx->sg_plaintext_num_elem;
+ sg = rec->sg_plaintext_data + rec->sg_plaintext_num_elem;
sg_set_page(sg, page, copy, offset);
sg_unmark_end(sg);
- ctx->sg_plaintext_num_elem++;
+ rec->sg_plaintext_num_elem++;
sk_mem_charge(sk, copy);
offset += copy;
size -= copy;
- ctx->sg_plaintext_size += copy;
- tls_ctx->pending_open_record_frags = ctx->sg_plaintext_num_elem;
+ rec->sg_plaintext_size += copy;
+ tls_ctx->pending_open_record_frags = rec->sg_plaintext_num_elem;
if (full_record || eor ||
- ctx->sg_plaintext_num_elem ==
- ARRAY_SIZE(ctx->sg_plaintext_data)) {
-push_record:
+ rec->sg_plaintext_num_elem ==
+ ARRAY_SIZE(rec->sg_plaintext_data)) {
ret = tls_push_record(sk, flags, record_type);
if (ret) {
- if (ret == -ENOMEM)
- goto wait_for_memory;
-
- goto sendpage_end;
+ if (ret == -EINPROGRESS)
+ num_async++;
+ else if (ret != -EAGAIN)
+ goto sendpage_end;
}
}
continue;
@@ -743,16 +993,20 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
wait_for_memory:
ret = sk_stream_wait_memory(sk, &timeo);
if (ret) {
- trim_both_sgl(sk, ctx->sg_plaintext_size);
+ trim_both_sgl(sk, rec->sg_plaintext_size);
goto sendpage_end;
}
- if (tls_is_pending_closed_record(tls_ctx))
- goto push_record;
-
goto alloc_payload;
}
+ if (num_async) {
+ /* Transmit if any encryptions have completed */
+ if (test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask)) {
+ cancel_delayed_work(&ctx->tx_work.work);
+ tls_tx_records(sk, flags);
+ }
+ }
sendpage_end:
if (orig_size > size)
ret = orig_size - size;
@@ -1300,6 +1554,49 @@ void tls_sw_free_resources_tx(struct sock *sk)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+ struct tls_rec *rec, *tmp;
+
+ /* Wait for any pending async encryptions to complete */
+ smp_store_mb(ctx->async_notify, true);
+ if (atomic_read(&ctx->encrypt_pending))
+ crypto_wait_req(-EINPROGRESS, &ctx->async_wait);
+
+ cancel_delayed_work_sync(&ctx->tx_work.work);
+
+ /* Tx whatever records we can transmit and abandon the rest */
+ tls_tx_records(sk, -1);
+
+ /* Free up un-sent records in tx_ready_list. First, free
+ * the partially sent record if any at head of tx_list.
+ */
+ if (tls_ctx->partially_sent_record) {
+ struct scatterlist *sg = tls_ctx->partially_sent_record;
+
+ while (1) {
+ put_page(sg_page(sg));
+ sk_mem_uncharge(sk, sg->length);
+
+ if (sg_is_last(sg))
+ break;
+ sg++;
+ }
+
+ tls_ctx->partially_sent_record = NULL;
+
+ rec = list_first_entry(&ctx->tx_ready_list,
+ struct tls_rec, list);
+ list_del(&rec->list);
+ kfree(rec);
+ }
+
+ list_for_each_entry_safe(rec, tmp, &ctx->tx_ready_list, list) {
+ free_sg(sk, rec->sg_encrypted_data,
+ &rec->sg_encrypted_num_elem,
+ &rec->sg_encrypted_size);
+
+ list_del(&rec->list);
+ kfree(rec);
+ }
crypto_free_aead(ctx->aead_send);
tls_free_both_sg(sk);
@@ -1336,6 +1633,24 @@ void tls_sw_free_resources_rx(struct sock *sk)
kfree(ctx);
}
+/* The work handler to transmitt the encrypted records in tx_ready_list */
+static void tx_work_handler(struct work_struct *work)
+{
+ struct delayed_work *delayed_work = to_delayed_work(work);
+ struct tx_work *tx_work = container_of(delayed_work,
+ struct tx_work, work);
+ struct sock *sk = tx_work->sk;
+ struct tls_context *tls_ctx = tls_get_ctx(sk);
+ struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
+
+ if (!test_and_clear_bit(BIT_TX_SCHEDULED, &ctx->tx_bitmask))
+ return;
+
+ lock_sock(sk);
+ tls_tx_records(sk, -1);
+ release_sock(sk);
+}
+
int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
{
struct tls_crypto_info *crypto_info;
@@ -1385,6 +1700,9 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
crypto_info = &ctx->crypto_send.info;
cctx = &ctx->tx;
aead = &sw_ctx_tx->aead_send;
+ INIT_LIST_HEAD(&sw_ctx_tx->tx_ready_list);
+ INIT_DELAYED_WORK(&sw_ctx_tx->tx_work.work, tx_work_handler);
+ sw_ctx_tx->tx_work.sk = sk;
} else {
crypto_init_wait(&sw_ctx_rx->async_wait);
crypto_info = &ctx->crypto_recv.info;
@@ -1435,26 +1753,6 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
goto free_iv;
}
- if (sw_ctx_tx) {
- sg_init_table(sw_ctx_tx->sg_encrypted_data,
- ARRAY_SIZE(sw_ctx_tx->sg_encrypted_data));
- sg_init_table(sw_ctx_tx->sg_plaintext_data,
- ARRAY_SIZE(sw_ctx_tx->sg_plaintext_data));
-
- sg_init_table(sw_ctx_tx->sg_aead_in, 2);
- sg_set_buf(&sw_ctx_tx->sg_aead_in[0], sw_ctx_tx->aad_space,
- sizeof(sw_ctx_tx->aad_space));
- sg_unmark_end(&sw_ctx_tx->sg_aead_in[1]);
- sg_chain(sw_ctx_tx->sg_aead_in, 2,
- sw_ctx_tx->sg_plaintext_data);
- sg_init_table(sw_ctx_tx->sg_aead_out, 2);
- sg_set_buf(&sw_ctx_tx->sg_aead_out[0], sw_ctx_tx->aad_space,
- sizeof(sw_ctx_tx->aad_space));
- sg_unmark_end(&sw_ctx_tx->sg_aead_out[1]);
- sg_chain(sw_ctx_tx->sg_aead_out, 2,
- sw_ctx_tx->sg_encrypted_data);
- }
-
if (!*aead) {
*aead = crypto_alloc_aead("gcm(aes)", 0, 0);
if (IS_ERR(*aead)) {
@@ -1491,6 +1789,8 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
sw_ctx_rx->sk_poll = sk->sk_socket->ops->poll;
strp_check_rcv(&sw_ctx_rx->strp);
+ } else {
+ ctx->tx_seq_number = be64_to_cpup((const __be64 *)rec_seq);
}
goto out;
--
2.13.6
^ permalink raw reply related
* 答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
From: Li,Rongqing @ 2018-09-21 3:27 UTC (permalink / raw)
To: Eric Dumazet, netdev@vger.kernel.org
In-Reply-To: <549f8bea-bd5f-70da-bbf3-c2331ffbf964@gmail.com>
: Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when
> sending to many devices
>
>
>
> On 09/20/2018 06:43 AM, Eric Dumazet wrote:
> >
>
Sorry, I should cc to you.
> > And lastly this patch looks way too complicated to me.
> > You probably can write something much simpler.
>
But it should not increase the negative performance
> Something like :
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index
> 930d17fa906c9ebf1cf7b6031ce0a22f9f66c0e4..e0a81beb4f37751421dbbe794c
> cf3d5a46bdf900 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -278,22 +278,26 @@ static bool netlink_filter_tap(const struct sk_buff
> *skb)
> return false;
> }
>
> -static int __netlink_deliver_tap_skb(struct sk_buff *skb,
> +static int __netlink_deliver_tap_skb(struct sk_buff **pskb,
> struct net_device *dev) {
> - struct sk_buff *nskb;
> + struct sk_buff *nskb, *skb = *pskb;
> struct sock *sk = skb->sk;
> int ret = -ENOMEM;
>
> if (!net_eq(dev_net(dev), sock_net(sk)))
> return 0;
>
> - dev_hold(dev);
> -
> - if (is_vmalloc_addr(skb->head))
> + if (is_vmalloc_addr(skb->head)) {
> nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
> - else
> - nskb = skb_clone(skb, GFP_ATOMIC);
> + if (!nskb)
> + return -ENOMEM;
> + consume_skb(skb);
The original skb can not be freed, since it will be used after send to tap in __netlink_sendskb
> + skb = nskb;
> + *pskb = skb;
> + }
> + dev_hold(dev);
> + nskb = skb_clone(skb, GFP_ATOMIC);
since original skb can not be freed, skb_clone will lead to a leak.
> if (nskb) {
> nskb->dev = dev;
> nskb->protocol = htons((u16) sk->sk_protocol); @@ -318,7 +322,7
> @@ static void __netlink_deliver_tap(struct sk_buff *skb, struct
> netlink_tap_net *n
> return;
>
> list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
> - ret = __netlink_deliver_tap_skb(skb, tmp->dev);
> + ret = __netlink_deliver_tap_skb(&skb, tmp->dev);
> if (unlikely(ret))
> break;
> }
>
The below change seems simple, but it increase skb allocation and
free one time,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e3a0538ec0be..b9631137f0fe 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -290,10 +290,8 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
dev_hold(dev);
- if (is_vmalloc_addr(skb->head))
- nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
- else
- nskb = skb_clone(skb, GFP_ATOMIC);
+ nskb = skb_clone(skb GFP_ATOMIC);
+
if (nskb) {
nskb->dev = dev;
nskb->protocol = htons((u16) sk->sk_protocol);
@@ -317,11 +315,20 @@ static void __netlink_deliver_tap(struct sk_buff *skb, struct netlink_tap_net *n
if (!netlink_filter_tap(skb))
return;
+ if (is_vmalloc_addr(skb->head)) {
+ skb = netlink_to_full_skb(skb, GFP_ATOMIC);
+ if (!skb)
+ return;
+ alloc = true;
+ }
+
list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
+
ret = __netlink_deliver_tap_skb(skb, tmp->dev);
if (unlikely(ret))
break;
}
+
+ if (alloc)
+ consume_skb(skb);
}
-Q
^ permalink raw reply related
* [PATCH] rsi: Remove unnecessary boolean condition
From: Nathan Chancellor @ 2018-09-21 9:48 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, netdev, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context.
drivers/net/wireless/rsi/rsi_91x_mac80211.c:927:50: warning: address of
array 'key->key' will always evaluate to 'true'
[-Wpointer-bool-conversion]
if (vif->type == NL80211_IFTYPE_STATION && key->key &&
~~ ~~~~~^~~
1 warning generated.
Link: https://github.com/ClangBuiltLinux/linux/issues/136
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/wireless/rsi/rsi_91x_mac80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 4e510cbe0a89..e56fc83faf0e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -924,7 +924,7 @@ static int rsi_hal_key_config(struct ieee80211_hw *hw,
if (status)
return status;
- if (vif->type == NL80211_IFTYPE_STATION && key->key &&
+ if (vif->type == NL80211_IFTYPE_STATION &&
(key->cipher == WLAN_CIPHER_SUITE_WEP104 ||
key->cipher == WLAN_CIPHER_SUITE_WEP40)) {
if (!rsi_send_block_unblock_frame(adapter->priv, false))
--
2.19.0
^ permalink raw reply related
* [PATCH] net/mlx4: Use cpumask_available for eq->affinity_mask
From: Nathan Chancellor @ 2018-09-21 9:44 UTC (permalink / raw)
To: Tariq Toukan, David S. Miller
Cc: netdev, linux-rdma, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context:
drivers/net/ethernet/mellanox/mlx4/eq.c:243:11: warning: address of
array 'eq->affinity_mask' will always evaluate to 'true'
[-Wpointer-bool-conversion]
if (!eq->affinity_mask || cpumask_empty(eq->affinity_mask))
~~~~~^~~~~~~~~~~~~
1 warning generated.
Use cpumask_available, introduced in commit f7e30f01a9e2 ("cpumask: Add
helper cpumask_available()"), which does the proper checking and avoids
this warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/86
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/ethernet/mellanox/mlx4/eq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 1f3372c1802e..2df92dbd38e1 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -240,7 +240,8 @@ static void mlx4_set_eq_affinity_hint(struct mlx4_priv *priv, int vec)
struct mlx4_dev *dev = &priv->dev;
struct mlx4_eq *eq = &priv->eq_table.eq[vec];
- if (!eq->affinity_mask || cpumask_empty(eq->affinity_mask))
+ if (!cpumask_available(eq->affinity_mask) ||
+ cpumask_empty(eq->affinity_mask))
return;
hint_err = irq_set_affinity_hint(eq->irq, eq->affinity_mask);
--
2.19.0
^ permalink raw reply related
* [PATCH] ath5k: Remove unused BUG_ON
From: Nathan Chancellor @ 2018-09-21 9:25 UTC (permalink / raw)
To: Jiri Slaby, Nick Kossifidis, Luis R. Rodriguez, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context:
drivers/net/wireless/ath/ath5k/debug.c:1031:14: warning: address of
array 'ah->sbands' will always evaluate to 'true'
[-Wpointer-bool-conversion]
BUG_ON(!ah->sbands);
~~~~~^~~~~~
./include/asm-generic/bug.h:61:45: note: expanded from macro 'BUG_ON'
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
^~~~~~~~~
./include/linux/compiler.h:77:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
1 warning generated.
Given that this condition is always false because of the logical not,
just remove it to fix the warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/142
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/wireless/ath/ath5k/debug.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index e01faf641288..94f70047d3fc 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -1028,8 +1028,6 @@ ath5k_debug_dump_bands(struct ath5k_hw *ah)
if (likely(!(ah->debug.level & ATH5K_DEBUG_DUMPBANDS)))
return;
- BUG_ON(!ah->sbands);
-
for (b = 0; b < NUM_NL80211_BANDS; b++) {
struct ieee80211_supported_band *band = &ah->sbands[b];
char bname[6];
--
2.19.0
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: emit RECORD_MMAP events for bpf prog load/unload
From: Alexei Starovoitov @ 2018-09-21 3:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, David S . Miller,
daniel, netdev, kernel-team
In-Reply-To: <20180920135651.GW24124@hirez.programming.kicks-ass.net>
On Thu, Sep 20, 2018 at 03:56:51PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 20, 2018 at 10:25:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > PeterZ provided a patch introducing PERF_RECORD_MUNMAP, went nowhere due
> > to having to cope with munmapping parts of existing mmaps, etc.
> >
> > I'm still more in favour of introduce PERF_RECORD_MUNMAP, even if for
> > now it would be used just in this clean case for undoing a
> > PERF_RECORD_MMAP for a BPF program.
> >
> > The ABI is already complicated, starting to use something called
> > PERF_RECORD_MMAP for unmmaping by just using a NULL name... too clever,
> > I think.
>
> Agreed, the PERF_RECORD_MUNMAP patch was fairly trivial, the difficult
> part was getting the perf tool to dtrt for that use-case. But if we need
> unmap events, doing the unmap record now is the right thing.
Thanks for the pointers!
The answer is a bit long. pls bear with me.
I have considered adding MUNMAP to match existing MMAP, but went
without it because I didn't want to introduce new bit in perf_event_attr
and emit these new events in a misbalanced conditional way for prog load/unload.
Like old perf is asking kernel for mmap events via mmap bit, so prog load events
will be in perf.data, but old perf report won't recognize them anyway.
Whereas new perf would certainly want to catch bpf events and will set
both mmap and mumap bits.
Then if I add MUNMAP event without new bit and emit MMAP/MUNMAP
conditionally based on single mmap bit they will confuse old perf
and it will print warning about 'unknown events'.
Both situations are ugly, hence I went with reuse of MMAP event
for both load/unload.
In such case old perf silently ignores them. Which is what I wanted.
When we upgrade the kernel we cannot synchronize the kernel upgrade
(or downgrade) with user space perf package upgrade.
Hence not confusing old perf is important.
With new kernel new bpf mmap events get into perf.data and
new perf picks them up.
Few more considerations:
I consider synthetic perf events to be non-ABI. Meaning they're
emitted by perf user space into perf.data and there is a convention
on names, but it's not a kernel abi. Like RECORD_MMAP with
event.filename == "[module_name]" is an indication for perf report
to parse elf/build-id of dso==module_name.
There is no such support in the kernel. Kernel doesn't emit
such events for module load/unload. If in the future
we decide to extend kernel with such events they don't have
to match what user space perf does today.
Why this is important? To get to next step.
As Arnaldo pointed out this patch set is missing support for
JITed prog annotations and displaying asm code. Absolutely correct.
This set only helps perf to reveal the names of bpf progs that _were_
running at the time of perf record, but there is no way yet for
perf report to show asm code of the prog that was running.
In that sense bpf is drastically different from java, other jits
and normal profiling.
bpf JIT happens in the kernel and only kernel knows the mapping
between original source and JITed code.
In addition there are bpf2bpf functions. In the future there will
be bpf libraries, more type info, line number support, etc.
I strongly believe perf RECORD_* events should NOT care about
the development that happens on the bpf side.
The only thing kernel will be telling user space is that bpf prog
with prog_id=X was loaded.
Then based on prog_id the 'perf record' phase can query the kernel
for bpf related information. There is already a way to fetch
JITed image based on prog_id.
Then perf will emit synthetic RECORD_FOOBAR into perf.data
that will contain bpf related info (like complete JITed image)
and perf report can process it later and annotate things in UI.
It may seem that there is a race here.
Like when 'perf record' see 'bpf prog was loaded with prog_id=X' event
it will ask the kernel about prog_id=X, but that prog could be
unloaded already.
In such case prog_id will not exist and perf record can ignore such event.
So no race.
The kernel doesn't need to pass all information about bpf prog to
the user space via RECORD_*. Instead 'perf record' can emit
synthetic events into perf.data.
I was thinking to extend RECORD_MMAP with prog_id already
(instead of passing kallsyms's bpf prog name in event->mmap.filename)
but bpf functions don't have their own prog_id. Multiple bpf funcs
with different JITed blobs are considered to be a part of single prog_id.
So as a step one I'm only extending RECORD_MMAP with addr and kallsym
name of bpf function/prog.
As a step two the plan is to add notification mechanism for prog load/unload
that will include prog_id and design new synthetic RECORD_* events in
perf user space that will contain JITed code, line info, BTF, etc.
TLDR:
step 1 (this patch set)
Single bpf prog_load can call multiple
bpf_prog_kallsyms_add() -> RECORD_MMAP with addr+kallsym only
Similarly unload calls multiple
bpf_prog_kallsyms_del() -> RECORD_MMAP with addr only
step 2 (future work)
single event for bpf prog_load with prog_id only.
Either via perf ring buffer or ftrace or tracepoints or some
other notification mechanism.
It may seem that step 2 obsoletes step 1. It can, but I think
it will complement it. There is a lot more code there and
a lot more discussions to have.
Step 1 is already big improvement.
Thoughts?
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in _decode_session6
From: Dmitry Vyukov @ 2018-09-21 8:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric Dumazet, syzbot, Alexei Starovoitov, Daniel Borkmann,
David Miller, Herbert Xu, Alexey Kuznetsov, LKML, netdev,
Steffen Klassert, syzkaller-bugs, Hideaki YOSHIFUJI
In-Reply-To: <CAADnVQL8=8O1W8-G-UP1akLijdGN82hnOJAY=sknTH_S0Y1rVQ@mail.gmail.com>
On Fri, Sep 21, 2018 at 8:21 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Sep 6, 2018 at 12:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>>> but I have a hard time reproducing the issue, so will appreciate
>>> if somebody can test the following patch:
>>
>> syzbot can:
>> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
>
> was the patch tested?
Hi Alexei,
syzbot tests patches on request. I don't see anybody requested any
testing for this bug. When testing is requested syzbot replies with
results generally within 30 mins. You can read more about patch
testing here:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
> it seems to me syzbot doesn't care about kernel quality but rather
> about the number of issues syzbot can find.
Finding and reporting bugs is a prerequisite for fixing them and
improving kernel quality. syzbot simply automates that part of bug
handling process, something that otherwise would needed to be done by
kernel developers. But active developer involvement and interest are
still required as not all parts are automatable.
^ permalink raw reply
* Re: [PATCH net-next 0/3] net: wean netfilter from fib_nh
From: David Miller @ 2018-09-21 3:02 UTC (permalink / raw)
To: dsahern; +Cc: netdev, netfilter-devel, pablo, fw, dsahern
In-Reply-To: <20180920205049.15143-1-dsahern@kernel.org>
From: dsahern@kernel.org
Date: Thu, 20 Sep 2018 13:50:46 -0700
> From: David Ahern <dsahern@gmail.com>
>
> Two netfilter modules reference fib_nh. In both cases the code is
> only checking if a nexthop in a fib_info uses a specific device.
> Both instances essentially duplicate code from __fib_validate_source,
> so move that code into a helper and flip the netfilter modules to
> use it.
Series applied, thanks David.
^ permalink raw reply
* Re: [PATCH net] r8169: fix autoneg issue on resume with RTL8168E
From: David Miller @ 2018-09-21 2:59 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev, neil
In-Reply-To: <ad74f3bd-ff60-1c8e-81cd-44ab76f942de@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 20 Sep 2018 22:47:09 +0200
> It was reported that chip version 33 (RTL8168E) ends up with
> 10MBit/Half on a 1GBit link after resuming from S3 (with different
> link partners). For whatever reason the PHY on this chip doesn't
> properly start a renegotiation when soft-reset.
> Explicitly requesting a renegotiation fixes this.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Fixes: a2965f12fde6 ("r8169: remove rtl8169_set_speed_xmii")
> Reported-by: Neil MacLeod <neil@nmacleod.com>
> Tested-by: Neil MacLeod <neil@nmacleod.com>
Applied, thank you.
Please always make "Fixes: " the first tag in a set of tags.
I fixed it up for you this time.
Thanks.
^ permalink raw reply
* [PATCH RESEND] PCI: hv: Fix return value check in hv_pci_assign_slots()
From: Wei Yongjun @ 2018-09-21 2:53 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Lorenzo Pieralisi, Bjorn Helgaas
Cc: Wei Yongjun, devel, linux-pci, netdev, kernel-janitors
In-Reply-To: <1537425631-28460-1-git-send-email-weiyongjun1@huawei.com>
In case of error, the function pci_create_slot() returns ERR_PTR() and
never returns NULL. The NULL test in the return value check should be
replaced with IS_ERR().
Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot information")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
Since the orig patch is merged from net tree, cc netdev@vger.kernel.org
---
drivers/pci/controller/pci-hyperv.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index ee80e79..9ba4d12 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1484,8 +1484,10 @@ static void hv_pci_assign_slots(struct hv_pcibus_device *hbus)
snprintf(name, SLOT_NAME_SIZE, "%u", hpdev->desc.ser);
hpdev->pci_slot = pci_create_slot(hbus->pci_bus, slot_nr,
name, NULL);
- if (!hpdev->pci_slot)
+ if (IS_ERR(hpdev->pci_slot)) {
pr_warn("pci_create slot %s failed\n", name);
+ hpdev->pci_slot = NULL;
+ }
}
}
^ permalink raw reply related
* Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array
From: Marek Szyprowski @ 2018-09-21 8:18 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Andrew Lunn, Ulf Hansson, linux-doc, linux-iio, Linus Walleij,
Dominik Brodowski, Peter Rosin, netdev, linux-i2c,
Peter Meerwald-Stadler, devel, Florian Fainelli, Jonathan Corbet,
Krzysztof Kozlowski, Kishon Vijay Abraham I, Tony Lindgren,
Lukas Wunner, Geert Uytterhoeven, linux-serial, Jiri Slaby,
Michael Hennerich, Uwe Kleine-König, linux-gpio,
Russell King
In-Reply-To: <15226900.TQMLYV7PZ0@z50>
Hi Janusz,
On 2018-09-20 18:21, Janusz Krzysztofik wrote:
> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may contain
>>>> information on direct mapping of array members to pins of a single GPIO
>>>> chip in hardware order. In such cases, bitmaps of values can be passed
>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>> wasting time on iterations.
>>>>
>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>>>> Pins not applicable for fast path are processed as before, skipping
>>>> over the 'fast' ones.
>>>>
>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>> I've just noticed that this patch landed in today's linux-next. Sadly it
>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>
>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
>>> boot. I will try later to add some debugs and investigate it further what
>>> really happens when booting hangs.
>> Hi Marek,
>>
>> Thanks for reporting. Could you please try the following fix?
> Hi again,
>
> I realized the patch was not correct, j, not i, should be updated in second
> hunk. Please try the following one.
>
> Thanks,
> Janusz
>
> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Date: Thu, 20 Sep 2018 17:37:21 +0200
> Subject: [PATCH] gpiolib: Fix bitmap index not updated
> While skipping fast path bits, bitmap index is not updated with next
> found zero bit position. Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
This one also doesn't help. A quick compare of logs with this version and
a working system shows, that with your patch (and fix) there are no calls to
gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
you need any more information (what kind of logs will help?), let me know.
> ---
> drivers/gpio/gpiolib.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a53d17745d21..369bdd358fcc 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> __set_bit(hwgpio, mask);
>
> if (array_info)
> - find_next_zero_bit(array_info->get_mask,
> + i = find_next_zero_bit(array_info->get_mask,
> array_size, i);
> else
> i++;
> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> trace_gpio_value(desc_to_gpio(desc), 1, value);
>
> if (array_info)
> - find_next_zero_bit(array_info->get_mask, i, j);
> + j = find_next_zero_bit(array_info->get_mask, i,
> + j);
> else
> j++;
> }
> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
> }
>
> if (array_info)
> - find_next_zero_bit(array_info->set_mask,
> + i = find_next_zero_bit(array_info->set_mask,
> array_size, i);
> else
> i++;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: add a missing error return
From: Jason Wang @ 2018-09-21 2:29 UTC (permalink / raw)
To: Dan Carpenter, Michael S. Tsirkin
Cc: kvm, virtualization, netdev, kernel-janitors
In-Reply-To: <20180920100158.GA9551@mwanda>
On 2018年09月20日 18:01, Dan Carpenter wrote:
> We accidentally left out this error return so it leads to some use after
> free bugs later on.
>
> Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index dd4e0a301635..1bff6bc8161a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1244,6 +1244,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> kfree(vqs);
> kvfree(n);
> kfree(queue);
> + return -ENOMEM;
> }
> n->vqs[VHOST_NET_VQ_TX].xdp = xdp;
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks!
^ permalink raw reply
* RE: [PATCH 00/21] SMMU enablement for NXP LS1043A and LS1046A
From: Laurentiu Tudor @ 2018-09-21 7:32 UTC (permalink / raw)
To: Leo Li
Cc: robin.murphy@arm.com,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Netdev, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Madalin-cristian Bucur, Roy Pledge, Shawn Guo, David Miller
In-Reply-To: <CADRPPNTot7U6vARqtrRrw4XqhDQPSXP1QO9ZbGpNQByJPcU37Q@mail.gmail.com>
> -----Original Message-----
> From: Li Yang [mailto:leoyang.li@nxp.com]
> Sent: Thursday, September 20, 2018 10:07 PM
>
> On Thu, Sep 20, 2018 at 5:39 AM Laurentiu Tudor <laurentiu.tudor@nxp.com>
> wrote:
> >
> >
> >
> > On 19.09.2018 17:37, Robin Murphy wrote:
> > > On 19/09/18 15:18, Laurentiu Tudor wrote:
> > >> Hi Robin,
> > >>
> > >> On 19.09.2018 16:25, Robin Murphy wrote:
> > >>> Hi Laurentiu,
> > >>>
> > >>> On 19/09/18 13:35, laurentiu.tudor@nxp.com wrote:
> > >>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > >>>>
> > >>>> This patch series adds SMMU support for NXP LS1043A and LS1046A
> chips
> > >>>> and consists mostly in important driver fixes and the required
> device
> > >>>> tree updates. It touches several subsystems and consists of three
> main
> > >>>> parts:
> > >>>> - changes in soc/drivers/fsl/qbman drivers adding iommu mapping
> of
> > >>>> reserved memory areas, fixes and defered probe support
> > >>>> - changes in drivers/net/ethernet/freescale/dpaa_eth drivers
> > >>>> consisting in misc dma mapping related fixes and probe
> ordering
> > >>>> - addition of the actual arm smmu device tree node together with
> > >>>> various adjustments to the device trees
> > >>>>
> > >>>> Performance impact
> > >>>>
> > >>>> Running iperf benchmarks in a back-to-back setup (both sides
> > >>>> having smmu enabled) on a 10GBps port show an important
> > >>>> networking performance degradation of around %40 (9.48Gbps
> > >>>> linerate vs 5.45Gbps). If you need performance but without
> > >>>> SMMU support you can use "iommu.passthrough=1" to disable
> > >>>> SMMU.
> > >>>>
> > >>>> USB issue and workaround
> > >>>>
> > >>>> There's a problem with the usb controllers in these chips
> > >>>> generating smaller, 40-bit wide dma addresses instead of the
> > >>>> 48-bit
> > >>>> supported at the smmu input. So you end up in a situation
> > >>>> where the
> > >>>> smmu is mapped with 48-bit address translations, but the
> device
> > >>>> generates transactions with clipped 40-bit addresses, thus
> smmu
> > >>>> context faults are triggered. I encountered a similar
> > >>>> situation for
> > >>>> mmc that I managed to fix in software [1] however for USB I
> > >>>> did not
> > >>>> find a proper place in the code to add a similar fix. The
> only
> > >>>> workaround I found was to add this kernel parameter which
> > >>>> limits the
> > >>>> usb dma to 32-bit size: "xhci-hcd.quirks=0x800000".
> > >>>> This workaround if far from ideal, so any suggestions for a
> code
> > >>>> based workaround in this area would be greatly appreciated.
> > >>>
> > >>> If you have a nominally-64-bit device with a
> > >>> narrower-than-the-main-interconnect link in front of it, that should
> > >>> already be fixed in 4.19-rc by bus_dma_mask picking up DT dma-
> ranges,
> > >>> provided the interconnect hierarchy can be described appropriately
> (or
> > >>> at least massaged sufficiently to satisfy the binding), e.g.:
> > >>>
> > >>> / {
> > >>> ...
> > >>>
> > >>> soc {
> > >>> ranges;
> > >>> dma-ranges = <0 0 10000 0>;
> > >>>
> > >>> dev_48bit { ... };
> > >>>
> > >>> periph_bus {
> > >>> ranges;
> > >>> dma-ranges = <0 0 100 0>;
> > >>>
> > >>> dev_40bit { ... };
> > >>> };
> > >>> };
> > >>> };
> > >>>
> > >>> and if that fails to work as expected (except for PCI hosts where
> > >>> handling dma-ranges properly still needs sorting out), please do let
> us
> > >>> know ;)
> > >>>
> > >>
> > >> Just to confirm, Is this [1] the change I was supposed to test?
> > >
> > > Not quite - dma-ranges is only valid for nodes representing a bus, so
> > > putting it directly in the USB device nodes doesn't work (FWIW that's
> > > why PCI is broken, because the parser doesn't expect the
> > > bus-as-leaf-node case). That's teh point of that intermediate simple-
> bus
> > > node represented by "periph_bus" in my example (sorry, I should have
> put
> > > compatibles in to make it clearer) - often that's actually true to
> life
> > > (i.e. "soc" is something like a CCI and "periph_bus" is something like
> > > an AXI NIC gluing a bunch of lower-bandwidth DMA masters to one of the
> > > CCI ports) but at worst it's just a necessary evil to make the binding
> > > happy (if it literally only represents the point-to-point link between
> > > the device master port and interconnect slave port).
> > >
> >
> > Quick update: so I adjusted to device tree according to your example and
> > it works so now I can get rid of that nasty kernel arg based workaround,
> > yey! :-)
>
> Great that we have a generic solution like I hoped for! So you will
> submit a new revision of the series to include these dts updates,
> right?
>
Yes, I already have it prepared. Just delaying the v2 for a few days maybe there will be some more feedback.
---
Best Regards, Laurentiu
^ permalink raw reply
* Re: [PATCH net-next 17/22] hv_netvsc: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 1:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: davem, dmitry.tarnyagin, wg, mkl, michal.simek, hsweeten,
madalin.bucur, pantelis.antoniou, claudiu.manoil, leoyang.li,
linux, sammy, ralf, nico, steve.glendinning, f.fainelli,
grygorii.strashko, w-kwok2, m-karicheri2, t.sailer, jreuter, kys,
haiyangz, wei.liu2, paul.durrant, arvid.brodin, pshelar, dev,
linux-mips, xen-devel, netdev, linux-usb
In-Reply-To: <20180920074341.3acef75c@xeon-e3>
On 2018/9/20 22:43, Stephen Hemminger wrote:
> On Thu, 20 Sep 2018 20:33:01 +0800
> YueHaibing <yuehaibing@huawei.com> wrote:
>
>> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
>> which is a typedef for an enum type, so make sure the implementation in
>> this driver has returns 'netdev_tx_t' value, and change the function
>> return type to netdev_tx_t.
>>
>> Found by coccinelle.
>>
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 3af6d8d..056c472 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -511,7 +511,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
>> return rc;
>> }
>>
>> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> +static netdev_tx_t
>> +netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> {
>> struct net_device_context *net_device_ctx = netdev_priv(net);
>> struct hv_netvsc_packet *packet = NULL;
>> @@ -528,8 +529,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> */
>> vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
>> if (vf_netdev && netif_running(vf_netdev) &&
>> - !netpoll_tx_running(net))
>> - return netvsc_vf_xmit(net, vf_netdev, skb);
>> + !netpoll_tx_running(net)) {
>> + ret = netvsc_vf_xmit(net, vf_netdev, skb);
>> + if (ret)
>> + return NETDEV_TX_BUSY;
>> + }
>
> Sorry, the new code is wrong. It will fall through if ret == 0 (NETDEV_TX_OK)
> Please review and test your patches.
I'm sorry for this, will correct it as Haiyang's suggestion.
>
> .
>
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-21 7:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <028b4cea-0e3f-fab5-7a74-cf003bbd1134@gmail.com>
> On Sep 20, 2018, at 4:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 04:43 PM, Song Liu wrote:
>>
>
>> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
>> the issue. However, netpoll will drop (fail to send) more packets.
>>
>
> Why is it failing ?
>
> If you are under high memory pressure, then maybe if you absolutely want memory to send
> netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
> from feeding incoming packets to the host and add more memory pressure ;)
>
I did the test with Eric's latest patch (and disable ndo_poll_controller
in driver). The result didn't show significant increase in drop packets.
I guess packet drops in my earlier test was caused by some other changes
I mixed there.
So I think this patch does fix the issue. Thanks Eric!
For ixgbe, I think we need to check napi_complete_done() return value
anyway. Otherwise, the driver will enable IRQ in polling mode.
Song
^ permalink raw reply
* RE: [PATCH net-next] net/tls: Add support for async encryption of records for performance
From: Vakul Garg @ 2018-09-21 1:14 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
davejwatson@fb.com, doronrk@fb.com
In-Reply-To: <20180920.111833.80229845984902983.davem@davemloft.net>
> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, September 20, 2018 11:49 PM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] net/tls: Add support for async encryption of
> records for performance
>
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Wed, 19 Sep 2018 20:51:35 +0530
>
> > This patch enables encryption of multiple records in parallel when an
> > async capable crypto accelerator is present in system.
>
> This seems to be trading off zero copy with async support.
>
> Async crypto device support is not the common case at all, and synchronous
> crypto via cpu crypto acceleration instructions is so much more likely.
>
> Oh I see, the new logic is only triggered with ASYNC_CAPABLE is set?
>
> > +static inline bool is_tx_ready(struct tls_context *tls_ctx,
> > + struct tls_sw_context_tx *ctx)
> > +{
>
> Two space between "inline" and "bool", please make it one.
Fixed.
Seems checkpatch misses it.
>
> > static void tls_write_space(struct sock *sk) {
> > struct tls_context *ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>
> Longest to shortest line (reverse christmas tree) ordering for local variable
> declarations please.
Can't do this. The second variable assignment is dependent upon previous one.
> >
> > + list_for_each_prev(pos, &ctx->tx_ready_list) {
> > + struct tls_rec *rec = (struct tls_rec *)pos;
> > + u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
>
> Likewise.
>
I can split variable declaration 'seq' and its assignment into two separate lines.
But I am not sure if increasing number of lines in order to comply reverse Christmas tree
is a good thing for this case.
> > -static int tls_do_encryption(struct tls_context *tls_ctx,
> > +int tls_tx_records(struct sock *sk, int flags) {
> > + struct tls_rec *rec, *tmp;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + int rc = 0;
> > + int tx_flags;
>
> Likewise.
Could partially address since ctx assignment depends upon tls_ctx assignment.
>
> > +static void tls_encrypt_done(struct crypto_async_request *req, int
> > +err) {
> > + struct aead_request *aead_req = (struct aead_request *)req;
> > + struct sock *sk = req->data;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
> > + int pending;
> > + bool ready = false;
>
> Likewise.
Placed 'ready' above pending 'pending'. Rest unchanged because of dependencies.
>
> > +static int tls_do_encryption(struct sock *sk,
> > + struct tls_context *tls_ctx,
> > struct tls_sw_context_tx *ctx,
> > struct aead_request *aead_req,
> > size_t data_len)
> > {
> > int rc;
> > + struct tls_rec *rec = ctx->open_rec;
>
> Likewise.
>
> > @@ -473,11 +630,12 @@ static int memcopy_from_iter(struct sock *sk,
> > struct iov_iter *from, {
> > struct tls_context *tls_ctx = tls_get_ctx(sk);
> > struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > - struct scatterlist *sg = ctx->sg_plaintext_data;
> > + struct tls_rec *rec = ctx->open_rec;
> > + struct scatterlist *sg = rec->sg_plaintext_data;
> > int copy, i, rc = 0;
>
> Likewise.
Can't change because of dependencies.
>
> > +struct tls_rec *get_rec(struct sock *sk) {
> > + int mem_size;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
>
> Likewise.
>
Declared 'mem_size' below 'rec'.
> > @@ -510,21 +707,33 @@ int tls_sw_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > int record_room;
> > bool full_record;
> > int orig_size;
> > + struct tls_rec *rec;
> > bool is_kvec = msg->msg_iter.type & ITER_KVEC;
> > + struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
> > + bool async_capable = tfm->__crt_alg->cra_flags &
> CRYPTO_ALG_ASYNC;
> > + int num_async = 0;
> > + int num_zc = 0;
>
> Likewise.
Fixed
> > @@ -661,6 +904,8 @@ int tls_sw_sendpage(struct sock *sk, struct page
> *page,
> > struct scatterlist *sg;
> > bool full_record;
> > int record_room;
> > + struct tls_rec *rec;
> > + int num_async = 0;
>
> Likewise.
Fixed.
Sending v2.
^ permalink raw reply
* Re: [PATCH iproute2-next] iplink: add ipvtap support
From: David Ahern @ 2018-09-21 0:55 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Stephen Hemminger, Phil Sutter, Sainath Grandhi, Davide Caratti
In-Reply-To: <1537326209-30837-1-git-send-email-liuhangbin@gmail.com>
On 9/18/18 8:03 PM, Hangbin Liu wrote:
> IPVLAN and IPVTAP are using the same functions and parameters. So we can
> just add a new link_util with id ipvtap. Others are the same.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> ip/iplink.c | 4 ++--
> ip/iplink_ipvlan.c | 28 ++++++++++++++++++----------
> man/man8/ip-link.8.in | 4 ++++
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
applied to iproute2-next. Thanks
^ permalink raw reply
* [PATCH net] net/ipv6: Display all addresses in output of /proc/net/if_inet6
From: Jeff Barnhill @ 2018-09-21 0:45 UTC (permalink / raw)
To: netdev; +Cc: davem, kuznet, yoshfuji, Jeff Barnhill
The backend handling for /proc/net/if_inet6 in addrconf.c doesn't properly
handle starting/stopping the iteration. The problem is that at some point
during the iteration, an overflow is detected and the process is
subsequently stopped. The item being shown via seq_printf() when the
overflow occurs is not actually shown, though. When start() is
subsequently called to resume iterating, it returns the next item, and
thus the item that was being processed when the overflow occurred never
gets printed.
Alter the meaning of the private data member "offset". Currently, when it
is not 0 (which only happens at the very beginning), "offset" represents
the next hlist item to be printed. After this change, "offset" always
represents the current item.
This is also consistent with the private data member "bucket", which
represents the current bucket, and also the use of "pos" as defined in
seq_file.txt:
The pos passed to start() will always be either zero, or the most
recent pos used in the previous session.
Signed-off-by: Jeff Barnhill <0xeffeff@gmail.com>
---
net/ipv6/addrconf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d51a8c0b3372..c63ccce6425f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4201,7 +4201,6 @@ static struct inet6_ifaddr *if6_get_first(struct seq_file *seq, loff_t pos)
p++;
continue;
}
- state->offset++;
return ifa;
}
@@ -4225,13 +4224,12 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
return ifa;
}
+ state->offset = 0;
while (++state->bucket < IN6_ADDR_HSIZE) {
- state->offset = 0;
hlist_for_each_entry_rcu(ifa,
&inet6_addr_lst[state->bucket], addr_lst) {
if (!net_eq(dev_net(ifa->idev->dev), net))
continue;
- state->offset++;
return ifa;
}
}
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the vSwitch
From: Stephen Hemminger @ 2018-09-21 0:41 UTC (permalink / raw)
To: Haiyang Zhang
Cc: davem@davemloft.net, netdev@vger.kernel.org, olaf@aepfle.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
vkuznets
In-Reply-To: <BN6PR21MB01614DCE1099980A7B1286FFCA130@BN6PR21MB0161.namprd21.prod.outlook.com>
On Thu, 20 Sep 2018 20:56:46 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, September 20, 2018 4:48 PM
> > To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> > netdev@vger.kernel.org; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; vkuznets <vkuznets@redhat.com>
> > Subject: Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the
> > vSwitch
> >
> > On Thu, 20 Sep 2018 17:06:59 +0000
> > Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> >
> > > +static inline void rsc_add_data
> > > + (struct netvsc_channel *nvchan,
> > > + const struct ndis_pkt_8021q_info *vlan,
> > > + const struct ndis_tcp_ip_checksum_info *csum_info,
> > > + void *data, u32 len)
> > > +{
> >
> > Could this be changed to look more like a function and skip the inline.
> > The compiler will end up inlining it anyway.
> >
> > static void rsc_add_data(struct netvsc_channel *nvchan,
>
> How about this?
> static inline
> void rsc_add_data(struct netvsc_channel *nvchan,
>
Sure that matches other code in that file
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in _decode_session6
From: Alexei Starovoitov @ 2018-09-21 6:21 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Eric Dumazet, syzbot, Alexei Starovoitov, Daniel Borkmann,
David Miller, Herbert Xu, Alexey Kuznetsov, LKML, netdev,
Steffen Klassert, syzkaller-bugs, Hideaki YOSHIFUJI
On Thu, Sep 6, 2018 at 12:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> but I have a hard time reproducing the issue, so will appreciate
>> if somebody can test the following patch:
>
> syzbot can:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
was the patch tested?
it seems to me syzbot doesn't care about kernel quality but rather
about the number of issues syzbot can find.
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Eric Dumazet @ 2018-09-20 23:49 UTC (permalink / raw)
To: Song Liu, Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <E4A7BD0D-B091-4FBD-94A9-F0104729DCF6@fb.com>
On 09/20/2018 04:43 PM, Song Liu wrote:
>
> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
> the issue. However, netpoll will drop (fail to send) more packets.
>
Why is it failing ?
If you are under high memory pressure, then maybe if you absolutely want memory to send
netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
from feeding incoming packets to the host and add more memory pressure ;)
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-20 23:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <ee9b9a55-c967-cec3-7df1-79f84b06154c@gmail.com>
> On Sep 20, 2018, at 4:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 03:42 PM, Song Liu wrote:
>>
>>
>>> On Sep 20, 2018, at 2:01 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>>>
>>> On Thu, 2018-09-20 at 13:35 -0700, Eric Dumazet wrote:
>>>> On 09/20/2018 12:01 PM, Song Liu wrote:
>>>>> The NIC driver should only enable interrupts when napi_complete_done()
>>>>> returns true. This patch adds the check for ixgbe.
>>>>>
>>>>> Cc: stable@vger.kernel.org # 4.10+
>>>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>>>> Suggested-by: Eric Dumazet <edumazet@google.com>
>>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>>> ---
>>>>
>>>>
>>>> Well, unfortunately we do not know why this is needed,
>>>> this is why I have not yet sent this patch formally.
>>>>
>>>> netpoll has correct synchronization :
>>>>
>>>> poll_napi() places into napi->poll_owner current cpu number before
>>>> calling poll_one_napi()
>>>>
>>>> netpoll_poll_lock() does also use napi->poll_owner
>>>>
>>>> When netpoll calls ixgbe poll() method, it passed a budget of 0,
>>>> meaning napi_complete_done() is not called.
>>>>
>>>> As long as we can not explain the problem properly in the changelog,
>>>> we should investigate, otherwise we will probably see coming dozens of
>>>> patches
>>>> trying to fix a 'potential hazard'.
>>>
>>> Agreed, which is why I have our validation and developers looking into it,
>>> while we test the current patch from Song.
>>
>> I figured out what is the issue here. And I have a proposal to fix it. I
>> have verified that this fixes the issue in our tests. But Alexei suggests
>> that it may not be the right way to fix.
>>
>> Here is what happened:
>>
>> netpoll tries to send skb with netpoll_start_xmit(). If that fails, it
>> calls netpoll_poll_dev(), which calls ndo_poll_controller(). Then, in
>> the driver, ndo_poll_controller() calls napi_schedule() for ALL NAPIs
>> within the same NIC.
>>
>> This is problematic, because at the end napi_schedule() calls:
>>
>> ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>
>> which attached these NAPIs to softnet_data on THIS CPU. This is done
>> via napi->poll_list.
>>
>> Then suddenly ksoftirqd on this CPU owns multiple NAPIs. And it will
>> not give up the ownership until it calls napi_complete_done(). However,
>> for a very busy server, we usually use 16 CPUs to poll NAPI, so this
>> CPU can easily be overloaded. And as a result, each call of napi->poll()
>> will hit budget (of 64), and it will not call napi_complete_done(),
>> and the NAPI stays in the poll_list of this CPU.
>>
>> When this happens, the host usually cannot get out of this state until
>> we throttle/stop client traffic.
>>
>>
>> I am pretty confident this is what happened. Please let me know if
>> anything above doesn't make sense.
>>
>>
>> Here is my proposal to fix it: Instead of polling all NAPIs within one
>> NIC, I would have netpoll to only poll the NAPI that will free space
>> for netpoll_start_xmit(). I attached my two RFC patches to the end of
>> this email.
>>
>> I chatted with Alexei about this. He think polling only one NAPI may
>> not guarantee netpoll make progress with the TX queue we are aiming
>> for. Also, the bigger problem may be the fact that NAPIs could get
>> pinned to one CPU and cannot get released.
>>
>> At this point, I really don't know what is the best way to fix this.
>>
>> I will also work on a repro with netperf.
>
> Thanks !
>
>>
>> Please let me know your suggestions.
>>
>
> Yeah, maybe that NICs using NAPI could not provide an ndo_poll_controller() method at all,
> since it is very risky (potentially grab many NAPI, and end up in this locked situation)
>
> poll_napi() could attempt to free skbs one napi at a time,
> without the current cpu stealing all NAPI.
>
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 57557a6a950cc9cdff959391576a03381d328c1a..a992971d366090ba69d5c1af32eadd554d6880cf 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -205,13 +205,8 @@ static void netpoll_poll_dev(struct net_device *dev)
> }
>
> ops = dev->netdev_ops;
> - if (!ops->ndo_poll_controller) {
> - up(&ni->dev_lock);
> - return;
> - }
> -
> - /* Process pending work on NIC */
> - ops->ndo_poll_controller(dev);
> + if (ops->ndo_poll_controller)
> + ops->ndo_poll_controller(dev);
>
> poll_napi(dev);
>
I tried to totally skip ndo_poll_controller() here. It did avoid hitting
the issue. However, netpoll will drop (fail to send) more packets.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH] net: netronome: remove redundant continue
From: zhong jiang @ 2018-09-21 5:21 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, dirk.vandermerwe, simon.horman, netdev, linux-kernel
In-Reply-To: <20180920093857.6227710f@cakuba.netronome.com>
On 2018/9/21 0:38, Jakub Kicinski wrote:
> On Thu, 20 Sep 2018 16:02:21 +0800, zhong jiang wrote:
>> The continue will not truely skip any code. hence it is safe to
>> remove it.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> I think this came up during review at some point. I still prefer to
> keep the continue. The body of the loop performs initialization of
> objects, if an object is removed we shouldn't carry on with the body.
> It's easy to miss that the object got freed otherwise, there is no
> error being set and no warning printed...
IMO, we should bring it back when the case truely occur you have said. At present.
We should not take too much into account. Maybe it will not occur.
Thanks,
zhong jiang
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> index 0b1ac9c..50d7b58 100644
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> @@ -230,10 +230,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
>> ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>>
>> /* Kill the vNIC if app init marked it as invalid */
>> - if (nn->port && nn->port->type == NFP_PORT_INVALID) {
>> + if (nn->port && nn->port->type == NFP_PORT_INVALID)
>> nfp_net_pf_free_vnic(pf, nn);
>> - continue;
>> - }
>> }
>>
>> if (list_empty(&pf->vnics))
>
> .
>
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Andy Lutomirski @ 2018-09-21 4:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A. Donenfeld, Andrew Lunn, Arnd Bergmann, Eric Biggers,
LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAKv+Gu-sciHeWVij8hWGF78HV_CLddMv7Xdf1GRSY+6B2yt48A@mail.gmail.com>
> On Sep 20, 2018, at 9:30 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 20 September 2018 at 21:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Hi Andy,
>>
>>> On Fri, Sep 21, 2018 at 5:23 AM Andy Lutomirski <luto@amacapital.net> wrote:
>>> At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
>>>
>>> kernel_fpu_call(func, arg);
>>>
>>> And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
>>>
>>> I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
>>
>> Haha. That's fun, and maybe we'll do that at some point, but I have
>> some other reasons too for being on a workqueue now.
>>
>
> Kernel mode crypto is callable from any context, and SIMD can be used
> in softirq context on arm64 (and on x86, even from hardirq context
> IIRC if the interrupt is taken from userland), in which case we'd
> already be on the irq stack.
The x86_64 irq stack handles nesting already.
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: David Ahern @ 2018-09-20 22:53 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev@vger.kernel.org
In-Reply-To: <20180920140617.uybaha3n7iz6ckvn@breakpoint.cc>
On 9/20/18 7:06 AM, Florian Westphal wrote:
> David Ahern <dsahern@gmail.com> wrote:
>>> $ make O=kbuild/perf -j 24 -s
>>> In file included from /home/dsa/kernel-3.git/include/linux/kernel.h:10:0,
>>> from /home/dsa/kernel-3.git/include/linux/list.h:9,
>>> from /home/dsa/kernel-3.git/include/linux/module.h:9,
>>> from /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c:13:
>>> /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c: In function
>>> ‘xfrm_output_resume’:
>>> /home/dsa/kernel-3.git/include/linux/compiler.h:252:20: warning: array
>>> subscript is above array bounds [-Warray-bounds]
>>> __read_once_size(&(x), __u.__c, sizeof(x)); \
>
> Does this thing avoid the warning?
nope, still see it.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox