From nobody Thu Dec 21 15:53:56 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Sww2x4Q2nz54wPR; Thu, 21 Dec 2023 15:54:01 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Sww2w6f2Lz3JWj; Thu, 21 Dec 2023 15:54:00 +0000 (UTC) (envelope-from markjdb@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-qk1-x72e.google.com with SMTP id af79cd13be357-7811db57cb4so59129085a.0; Thu, 21 Dec 2023 07:54:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1703174040; x=1703778840; darn=freebsd.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :from:to:cc:subject:date:message-id:reply-to; bh=lAgGOBL3v9bbphEGuE/fAKgEPnyjUFURn9U+FBKtj78=; b=crW0hT886UhhtqTXg+f/8WjR2EdK2v4TU7dnDv4Hs/+OXIgMBctncvifdxT1vwUEYK QotZFQTypONg7xSp1iBflpB4oH7ELr5+LXBfpaMOlMP6N+ue/22RxrSD4GbrpUSUntil dcCwHdUrhVwH7Nc0V8cYFPgBF+EGBFfsEAGrRyxYHyuQdyOjiuyKLTWqW3usQAPur7uI uSCeJLiwXbxwyCvXSZQu7zvkdIIw3etquJOKBMzpOIxicobqb55FmMy6JcQo5QsjwiIm 5imKqffwrCX2jb5KqrGOJaTofkns0BPBEftfnfoMTSvucrk2ebi4qs2sXYxST7jXvqu9 5ixQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703174040; x=1703778840; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:sender :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=lAgGOBL3v9bbphEGuE/fAKgEPnyjUFURn9U+FBKtj78=; b=sgyDK05RpDiOU8Flvf4+IDTCOP93gV4QjxM9jF4HvwWo5CJN7Qcz2WIIJBen7Ur6jI fefZdgXu0IT8PzvxZOxtYfRHrSlQiD2/ae+Jpw5JiNVxXRABdTSrIrXhti6slKscYX1l F0tYFwfJ+gNRe9ydUCsc9fL/KgY6uazfUQuXOFnbfyOoa81OByoOPFxnpIgLbt0egSth x1MncDasb9TL+14tk8WSmCNQEdJetGnZTbpjWxan8lY+p5rGBz5OuhF11ARG78TNoR7r J74tGFSsnDT7gwXPzq36bZ0m9IspK/tu0Qw+21Mejl0bgz423sXaMzvAC/tkkKz7hmmX dXHQ== X-Gm-Message-State: AOJu0YzEjoVrYYZmIzbDSCIfEYYCel4bHVe6Fm5WTSa0uLNaKaOWHZss R5jcJM96ZW1/ukwRWSN1KODySIo9uQsfgw== X-Google-Smtp-Source: AGHT+IE6vSyPhZmxOgSNZ/aEoDeLyaa5MiXcGotOt7eNQcj1uO/furMqcKsXlC8hhu8lrAB4Gwr+qw== X-Received: by 2002:a05:620a:29d1:b0:77f:746d:2f61 with SMTP id s17-20020a05620a29d100b0077f746d2f61mr1425050qkp.1.1703174039781; Thu, 21 Dec 2023 07:53:59 -0800 (PST) Received: from nuc (192-0-220-237.cpe.teksavvy.com. [192.0.220.237]) by smtp.gmail.com with ESMTPSA id o4-20020a05620a110400b0078105826d55sm739933qkk.74.2023.12.21.07.53.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Dec 2023 07:53:58 -0800 (PST) Date: Thu, 21 Dec 2023 10:53:56 -0500 From: Mark Johnston To: Warner Losh Cc: Warner Losh , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 9e6d11ce9a51 - main - vtnet: Adjust rx buffer so IP header 32-bit aligned Message-ID: References: <202312210421.3BL4Lb9T036317@gitrepo.freebsd.org> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] X-Spamd-Bar: ---- X-Rspamd-Queue-Id: 4Sww2w6f2Lz3JWj On Thu, Dec 21, 2023 at 08:31:36AM -0700, Warner Losh wrote: > On Thu, Dec 21, 2023 at 8:13 AM Mark Johnston wrote: > > > On Thu, Dec 21, 2023 at 04:21:37AM +0000, Warner Losh wrote: > > > The branch main has been updated by imp: > > > > > > URL: > > https://cgit.FreeBSD.org/src/commit/?id=9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4 > > > > > > commit 9e6d11ce9a51d75ed6a94e180f2fb4e9188a2ba4 > > > Author: Warner Losh > > > AuthorDate: 2023-12-20 19:09:09 +0000 > > > Commit: Warner Losh > > > CommitDate: 2023-12-21 04:16:45 +0000 > > > > > > vtnet: Adjust rx buffer so IP header 32-bit aligned > > > > > > Call madj(m, ETHER_ALIGN) to offset rx buffers when allocating them. > > > This improves performance everywhere, and allows armv7 to work at > > all. > > > > > > PR: 271288 (PR had a different fix than I wound > > up with) > > > MFC After: 3 days > > > Sponsored by: Netflix > > > Differential Revision: https://reviews.freebsd.org/D43136 > > > --- > > > sys/dev/virtio/network/if_vtnet.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/sys/dev/virtio/network/if_vtnet.c > > b/sys/dev/virtio/network/if_vtnet.c > > > index 287af7751066..360176e4f845 100644 > > > --- a/sys/dev/virtio/network/if_vtnet.c > > > +++ b/sys/dev/virtio/network/if_vtnet.c > > > @@ -1532,8 +1532,8 @@ vtnet_rx_alloc_buf(struct vtnet_softc *sc, int > > nbufs, struct mbuf **m_tailp) > > > m_freem(m_head); > > > return (NULL); > > > } > > > - > > > m->m_len = size; > > > + m_adj(m, ETHER_ALIGN); > > > > The driver is expecting to get a cluster of size sc->vtnet_rx_clustersz, > > but now it's getting one of size sc->vtnet_rx_clustersz - 2. I don't > > see how this change can be sufficient on its own: what prevents virtio > > from writing sc->vtnet_rx_clustersz bytes and thereby overwriting the > > two bytes following the cluster? > > > > The only trouble that I saw was here: > > /* > * Every mbuf should have the expected cluster size since > that > * is also used to allocate the replacements. > */ > KASSERT(m->m_len == clustersz, > ("%s: mbuf size %d not expected cluster size %d", > __func__, > m->m_len, clustersz)); > > and even that's fine: We can adjust that assert. The next lines I think > make it fine: > m->m_len = MIN(m->m_len, len); > > so we only use what we say is there to land bytes into the mbuf. I'm now > not sure though what to do about a few lines later: > > m_new = vtnet_rx_alloc_buf(sc, nreplace, &m_tail); > if (m_new == NULL) { > m_prev->m_len = clustersz; > return (ENOBUFS); > } > > which likely needs to be adjusted (or the old saved size). I'm not certain about what it should be doing instead, but yes this code looks wrong now. > I likely didn't see asserts in my testing because I didn't have LRO slow > path packets. > > Do you see other places that would need adjusting? Presumably vtnet_rx_cluster_size() needs to be adjusted too? I also don't understand why we do this adjustment unconditionally instead of only when __NO_STRICT_ALIGNMENT is not defined, as almost all other NIC drivers do. > All the other places > seem to use m_len correctly on the rx path, but I'm not a nic driver guy, > and I might be missing something.