Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7a1716ca-365f-c869-3a57-94413234fb32@intel.com>
Date:   Mon, 22 May 2023 18:46:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH RESEND bpf-next 05/15] ice: Introduce ice_xdp_buff
Content-Language: en-US
To:     Larysa Zaremba <larysa.zaremba@intel.com>
CC:     <bpf@vger.kernel.org>, Stanislav Fomichev <sdf@google.com>,
        "Alexei Starovoitov" <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        "KP Singh" <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Tony Nguyen <anthony.l.nguyen@intel.com>,
        Anatoly Burakov <anatoly.burakov@intel.com>,
        Jesper Dangaard Brouer <brouer@redhat.com>,
        Alexander Lobakin <alexandr.lobakin@intel.com>,
        "Magnus Karlsson" <magnus.karlsson@gmail.com>,
        Maryam Tahhan <mtahhan@redhat.com>,
        <xdp-hints@xdp-project.net>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-6-larysa.zaremba@intel.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230512152607.992209-6-larysa.zaremba@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dlNqK1VIRllveWNKTHh3dUdGNDRQSEJyU2IxNGNIMmljb3RNTWFENlFraC94?=
 =?utf-8?B?WkNSa09iSCswNEZCOVZJNE45blpOQnBIcXVKaHFjendGSnlzRjlVYStYRHhy?=
 =?utf-8?B?RWgxMVZiWU81UjRDYS8rckw5VE93alpBWWNWSDJVSkNWSzYvRHFxMVFIOVBZ?=
 =?utf-8?B?QTdvTnV3S1pRWHRIYkxJdk1zbE1oUXQ3MXI3NnhxcE1sRkJMZVRvT21tNWZQ?=
 =?utf-8?B?YTlMcDc4anBnWWx1dzJPRzhSM0lsV1lESVl0aVhWdHlSWnYxbjgzaWhRa2NH?=
 =?utf-8?B?aEJpT2dFL2YrUFNWMDVXcSs5cjMwMFFwWmhudlVCQ2d4MkgwREZFNU5IaWtL?=
 =?utf-8?B?amRzeWNjRGJtU0R6dGFXdElSRUx3NXgySjJiNWt0cXpOeW1jSVFBOUR2NHV1?=
 =?utf-8?B?SUVzZjlZM0NBYWUvSFJ5bk5IaWgzLzlIVjZtaGpERjd2WkVDb094UXhaT1BB?=
 =?utf-8?B?bytEK1ZLb01UbmNkN2hIQmNrdStKVHZQdWNNbTREMjViSHNUSEx4L0sreHM5?=
 =?utf-8?B?dU5tMUZub2lNT0k1TU5wMktHOVFFZUt1ZDBUK1hqNkJzTUdGVExUT0NvWHlB?=
 =?utf-8?B?QkFXV00yOUN1dUxGRzJPZlh6YmpzSmUwVjJCN2RkdDVGOElvQmI5ak1TdnZt?=
 =?utf-8?B?bGRuZlNTRzc2VGtEbFp6dEJsQ1IwNzNkQjlNV3F4aExsdytGM0V2eS9kNUR4?=
 =?utf-8?B?QndMeTkxbGJLTlVyUXRMTkVvdXlRMWJkYTR4aEV0Tm5ZM0FmR1pWNFlIVi9k?=
 =?utf-8?B?ZzljMGFUNkMxZlJ2OVlsRlh3SWtLdGhnYUFJSHg3TERtQnkrM29ndXlTdHNV?=
 =?utf-8?B?T0ZYVjNZcjEzVUx0RDJKQ3BjeTUyRFZzVmpXekw4YytWZGNwTXVURTZjdkZ1?=
 =?utf-8?B?am52SGtwYksrTWI5U1pCbHB4VnBuRnY4ZHRXZEtvV1FpTDJJeFBlUlV0cTF1?=
 =?utf-8?B?NkZPMEdvcmo0WGFPclN5cWlMTFNDOTlxcmNsOW5jcUNiQkhqNE1DeCtxZmNj?=
 =?utf-8?B?ZFZpVGhiUFBRbkVnK05kR3ZnYWFhZzNXTzBQMzJCREs1ZnV1UWZ0VnlxK0d0?=
 =?utf-8?B?MGV3NVV5a0s2QWtOVXRKaW9ScHhyZTVla1RXQzROdzMxeGx0YThSQUsxSHdx?=
 =?utf-8?B?VHhsNTdMamcvQXVqQ3YxaUtxMWt1aC9HSUpCV0hRWmJiZEJpTHUxUWVOeU1v?=
 =?utf-8?B?M1RFRG9WOVIxeE9wMmcwWnB3RklLRHFpUDQveWU2TjcvUzlWWjVaRm9HUTEv?=
 =?utf-8?B?WGUwODBKYmNMNCtjM0w5c2lxeE1HL1M4UUh0MzNBYVV0SUx0MFV5K0NCclNZ?=
 =?utf-8?B?d241ZlVkUVV6UkEvWVgzbCs2c2VkWFJvVFRya0k5MDVkcGhsajl2ZHFyY3Y2?=
 =?utf-8?B?Qi9MeGhxOWt5U0V5d0crSlVWSGtocVYyMG83SDZtSUFpWEZUd1crN2JiWFhO?=
 =?utf-8?B?cUI4NWNHbWY3WXI0RldyNlVOb0xLL0hONnRvK1pxbWRUdmVCWTdRdk5wRkhh?=
 =?utf-8?B?NDJneEliL3ZhRi9iU3RIMGVqRFl1V245QTdVTXJpTDVJYU44WTlqaGw5RHNs?=
 =?utf-8?B?YkIyZVNNdHEyeXh6WUcyZVU1MTBqVGtIb1pFcG5rdDlucG15bGo0TWIzRCtJ?=
 =?utf-8?B?V081dTNncms0Z2RGdnByNFBTcUY3UXhDVEFyQTlEOXJHbXZ6WVlPbkZ5ZE9S?=
 =?utf-8?B?Z3liNVpUR0RvcStQQmZqbERINDZ0NmlXSSs4bVlhdHhhNUVUTGdiRTUrRVBx?=
 =?utf-8?B?TmZnRlBwRlV6NDh6U1VBb214WWIxWjJ0TDFHRURueEI0bFlCcHpwYzM3U1ZF?=
 =?utf-8?B?bXlpNE10TytOb2xNc2hRTU1DMGszTHVNdUNtL3didEI2WExoa0d5M3JhV2wx?=
 =?utf-8?B?YkNVampjYjhqNjl3NEQ0Z0tCSFIyRVdXQXIrUkdZMDhna0k4Mm15RFB3QXNp?=
 =?utf-8?B?bzMrZWtMdHlxY2RLUURNU0FNbzVPd3hta2p4bklCMVRJUzhsMEFONEg2ajI0?=
 =?utf-8?B?T2hGbmcxQ1ZwL0FDa0VST2FrSmwycmNzRnRwa0RralpXUXQvakhQQzlGSjlz?=
 =?utf-8?B?NUtsZyswUGErMmpCVlhpVFMybnR6L2ZlUGRkWWJpWGhDSkJ6SG1FMUhPZ1Fk?=
 =?utf-8?B?NllMTE1iUE1oTnFYMVAvdzZyaXpwbVVuZkVkQzRITzZvSUpFR0JuSFFrc2hJ?=
 =?utf-8?B?ZGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ec9c62f2-3ba0-4b7e-c87b-08db5ae455d0
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2023 16:48:14.5669
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: snk4bo5t+pfJFfR4d4/Uikb4/xwhA3xg+QdM0qzRYpzG2M7KIQzDeeRkM0KPFZOekbUG4MunC4buM6oLYDPFzy79xVPKJC0sP/xq8HZt/FY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB8490
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Larysa Zaremba <larysa.zaremba@intel.com>
Date: Fri, 12 May 2023 17:25:57 +0200

> In order to use XDP hints via kfuncs we need to put
> RX descriptor and ring pointers just next to xdp_buff.
> Same as in hints implementations in other drivers, we archieve
> this through putting xdp_buff into a child structure.
> 
> Currently, xdp_buff is stored in the ring structure,
> so replace it with union that includes child structure.
> This way enough memory is available while existing XDP code
> remains isolated from hints.
> 
> Size of the new child structure (ice_xdp_buff) is 72 bytes,
> therefore it does not fit into a single cache line.
> To at least place union at the start of cache line, move 'next'
> field from CL3 to CL1, as it isn't used often.
> 
> Placing union at the start of cache line makes at least xdp_buff
> and descriptor fit into a single CL,
> ring pointer is used less often, so it can spill into the next CL.

Spill or span?

> 
> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx.c     |  7 ++++--
>  drivers/net/ethernet/intel/ice/ice_txrx.h     | 23 ++++++++++++++++---
>  drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 11 +++++++++
>  3 files changed, 36 insertions(+), 5 deletions(-)

[...]

> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -260,6 +260,15 @@ enum ice_rx_dtype {
>  	ICE_RX_DTYPE_SPLIT_ALWAYS	= 2,
>  };
>  
> +struct ice_xdp_buff {
> +	struct xdp_buff xdp_buff;
> +	union ice_32b_rx_flex_desc *eop_desc;	/* Required for all metadata */

Probably can be const here as well after changing all the places
appropriately -- I don't think you write to it anywhere.

> +	/* End of the 1st cache line */
> +	struct ice_rx_ring *rx_ring;

Can't we get rid of ring dependency? Maybe there's only a couple fields
that could be copied here instead of referencing the ring? I just find
it weird that our drivers often look for something in the ring structure
to parse a descriptor ._.
If not, can't it be const?

> +};
> +
> +static_assert(offsetof(struct ice_xdp_buff, xdp_buff) == 0);
> +
>  /* indices into GLINT_ITR registers */
>  #define ICE_RX_ITR	ICE_IDX_ITR0
>  #define ICE_TX_ITR	ICE_IDX_ITR1
> @@ -301,7 +310,6 @@ enum ice_dynamic_itr {
>  /* descriptor ring, associated with a VSI */
>  struct ice_rx_ring {
>  	/* CL1 - 1st cacheline starts here */
> -	struct ice_rx_ring *next;	/* pointer to next ring in q_vector */
>  	void *desc;			/* Descriptor ring memory */
>  	struct device *dev;		/* Used for DMA mapping */
>  	struct net_device *netdev;	/* netdev ring maps to */
> @@ -313,12 +321,19 @@ struct ice_rx_ring {
>  	u16 count;			/* Number of descriptors */
>  	u16 reg_idx;			/* HW register index of the ring */
>  	u16 next_to_alloc;
> -	/* CL2 - 2nd cacheline starts here */
> +
>  	union {
>  		struct ice_rx_buf *rx_buf;
>  		struct xdp_buff **xdp_buf;
>  	};
> -	struct xdp_buff xdp;
> +	/* CL2 - 2nd cacheline starts here
> +	 * Size of ice_xdp_buff is 72 bytes,
> +	 * so it spills into CL3
> +	 */
> +	union {
> +		struct ice_xdp_buff xdp_ext;
> +		struct xdp_buff xdp;
> +	};

...or you can leave just one xdp_ext (naming it just "xdp") -- for now,
this union does literally nothing, as xdp_ext contains xdp at its very
beginning.

>  	/* CL3 - 3rd cacheline starts here */
>  	struct bpf_prog *xdp_prog;
>  	u16 rx_offset;
> @@ -328,6 +343,8 @@ struct ice_rx_ring {
>  	u16 next_to_clean;
>  	u16 first_desc;
>  
> +	struct ice_rx_ring *next;	/* pointer to next ring in q_vector */

It can be placed even farther, somewhere near rcu_head -- IIRC it's not
used anywhere on hotpath. Even ::ring_stats below is hotter.

> +
>  	/* stats structs */
>  	struct ice_ring_stats *ring_stats;
>  
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> index e1d49e1235b3..2835a8348237 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h
> @@ -151,4 +151,15 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring,
>  		       struct sk_buff *skb);
>  void
>  ice_receive_skb(struct ice_rx_ring *rx_ring, struct sk_buff *skb, u16 vlan_tag);
> +
> +static inline void
> +ice_xdp_set_meta_srcs(struct xdp_buff *xdp,

Not sure about the naming... But can't propose anything :clownface:
ice_xdp_init_buff()? Like xdp_init_buff(), but ice_xdp_buff :D

> +		      union ice_32b_rx_flex_desc *eop_desc,
> +		      struct ice_rx_ring *rx_ring)
> +{
> +	struct ice_xdp_buff *xdp_ext = (struct ice_xdp_buff *)xdp;

I'd use container_of(), even though it will do the same thing here.
BTW, is having &xdp_buff at offset 0 still a requirement?

> +
> +	xdp_ext->eop_desc = eop_desc;
> +	xdp_ext->rx_ring = rx_ring;
> +}
>  #endif /* !_ICE_TXRX_LIB_H_ */

Thanks,
Olek