diff mbox series

[RESEND,net-next,v4,4/4] sfc: remove expired unicast PTP filters

Message ID 20230314100925.12040-5-ihuguet@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series sfc: support unicast PTP | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: Reported-by: should be immediately followed by Link: with a URL to the report WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Íñigo Huguet March 14, 2023, 10:09 a.m. UTC 
Filters inserted to support unicast PTP mode might become unused after
some time, so we need to remove them to avoid accumulating many of them.

Actually, it would be a very unusual situation that many different
addresses are used, normally only a small set of predefined
addresses are tried. Anyway, some cleanup is necessary because
maintaining old filters forever makes very little sense.

Reported-by: Yalin Li <yalli@redhat.com>
Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
---
 drivers/net/ethernet/sfc/ptp.c | 83 ++++++++++++++++++++++++----------
 1 file changed, 60 insertions(+), 23 deletions(-)

Comments

Edward Cree March 14, 2023, 5:24 p.m. UTC | #1 
On 14/03/2023 10:09, Íñigo Huguet wrote:
> Filters inserted to support unicast PTP mode might become unused after
> some time, so we need to remove them to avoid accumulating many of them.
> 
> Actually, it would be a very unusual situation that many different
> addresses are used, normally only a small set of predefined
> addresses are tried. Anyway, some cleanup is necessary because
> maintaining old filters forever makes very little sense.
> 
> Reported-by: Yalin Li <yalli@redhat.com>
> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>

Acked-by: Edward Cree <ecree.xilinx@gmail.com>
but again a couple of nits to think about...

>  static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  {
> +	struct efx_ptp_data *ptp = efx->ptp_data;
>  	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
>  	struct efx_filter_spec spec;
>  
> @@ -1418,7 +1437,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>  	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
>  	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
>  	spec.ether_type = htons(ETH_P_1588);
> -	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
> +	return efx_ptp_insert_filter(efx, &ptp->rxfilters_mcast, &spec, 0);
>  }

If respinning for any reason, maybe move the addition of the
 local to patch #2.

> +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx)
> +{
> +	struct efx_ptp_data *ptp = efx->ptp_data;
> +	struct efx_ptp_rxfilter *rxfilter, *tmp;
> +
> +	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) {
> +		if (time_is_before_jiffies(rxfilter->expiry))
> +			efx_ptp_remove_one_filter(efx, rxfilter);
> +	}
> +}
> +
>  static int efx_ptp_start(struct efx_nic *efx)
>  {
>  	struct efx_ptp_data *ptp = efx->ptp_data;
> @@ -1631,6 +1666,8 @@ static void efx_ptp_worker(struct work_struct *work)
>  
>  	while ((skb = __skb_dequeue(&tempq)))
>  		efx_ptp_process_rx(efx, skb);
> +
> +	efx_ptp_drop_expired_unicast_filters(efx);
>  }
PTP worker runs on every PTP packet TX or RX, which might be
 quite frequent.  It's probably fine but do we need to consider
 limiting how much time we spend repeatedly scanning the list?
Conversely, if all PTP traffic suddenly stops, I think existing
 unicast filters will stay indefinitely.  Again probably fine
 but just want to check that sounds sane to everyone.
Íñigo Huguet March 15, 2023, 8:54 a.m. UTC | #2 
El 14/3/23 a las 18:24, Edward Cree escribió:
> On 14/03/2023 10:09, Íñigo Huguet wrote:
>> Filters inserted to support unicast PTP mode might become unused after
>> some time, so we need to remove them to avoid accumulating many of them.
>>
>> Actually, it would be a very unusual situation that many different
>> addresses are used, normally only a small set of predefined
>> addresses are tried. Anyway, some cleanup is necessary because
>> maintaining old filters forever makes very little sense.
>>
>> Reported-by: Yalin Li <yalli@redhat.com>
>> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> 
> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
> but again a couple of nits to think about...
> 
>>  static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>>  {
>> +	struct efx_ptp_data *ptp = efx->ptp_data;
>>  	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
>>  	struct efx_filter_spec spec;
>>  
>> @@ -1418,7 +1437,7 @@ static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
>>  	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
>>  	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
>>  	spec.ether_type = htons(ETH_P_1588);
>> -	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
>> +	return efx_ptp_insert_filter(efx, &ptp->rxfilters_mcast, &spec, 0);
>>  }
> 
> If respinning for any reason, maybe move the addition of the
>  local to patch #2.
> 
>> +static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx)
>> +{
>> +	struct efx_ptp_data *ptp = efx->ptp_data;
>> +	struct efx_ptp_rxfilter *rxfilter, *tmp;
>> +
>> +	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) {
>> +		if (time_is_before_jiffies(rxfilter->expiry))
>> +			efx_ptp_remove_one_filter(efx, rxfilter);
>> +	}
>> +}
>> +
>>  static int efx_ptp_start(struct efx_nic *efx)
>>  {
>>  	struct efx_ptp_data *ptp = efx->ptp_data;
>> @@ -1631,6 +1666,8 @@ static void efx_ptp_worker(struct work_struct *work)
>>  
>>  	while ((skb = __skb_dequeue(&tempq)))
>>  		efx_ptp_process_rx(efx, skb);
>> +
>> +	efx_ptp_drop_expired_unicast_filters(efx);
>>  }
> PTP worker runs on every PTP packet TX or RX, which might be
>  quite frequent.  It's probably fine but do we need to consider
>  limiting how much time we spend repeatedly scanning the list?

PTP traffic is not that frequent, few packets per second, isn't it?

> Conversely, if all PTP traffic suddenly stops, I think existing
>  unicast filters will stay indefinitely.  Again probably fine
>  but just want to check that sounds sane to everyone.

Yes, it's as you say. However, I thought it didn't worth it to create a new periodic worker only for this, given that I expected a short list, it wouldn't be harmful. However, as I said in the other message, maybe the list can be quite long if we're the PTP master?

Maybe I should create a dedicated periodic work for this? That would avoid both problems that you are pointing out.
Edward Cree March 15, 2023, 4:17 p.m. UTC | #3 
On 15/03/2023 08:54, Íñigo Huguet wrote:
>> PTP worker runs on every PTP packet TX or RX, which might be
>>  quite frequent.  It's probably fine but do we need to consider
>>  limiting how much time we spend repeatedly scanning the list?
> 
> PTP traffic is not that frequent, few packets per second, isn't it?
> 
>> Conversely, if all PTP traffic suddenly stops, I think existing
>>  unicast filters will stay indefinitely.  Again probably fine
>>  but just want to check that sounds sane to everyone.
> 
> Yes, it's as you say. However, I thought it didn't worth it to create a new periodic worker only for this, given that I expected a short list, it wouldn't be harmful. However, as I said in the other message, maybe the list can be quite long if we're the PTP master?
> 
> Maybe I should create a dedicated periodic work for this? That would avoid both problems that you are pointing out.

I'm not a PTP expert, hence why my comments above were phrased as
 questions rather than answers ;)
Up to you whether you think a dedicated work is needed; again, my
 Ack stands as it is, I'm happy for this to be done in a followup
 or not at all.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index 16686aa5bfb4..1447683dce31 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -75,6 +75,9 @@ 
 /* How long an unmatched event or packet can be held */
 #define PKT_EVENT_LIFETIME_MS		10
 
+/* How long unused unicast filters can be held */
+#define UCAST_FILTER_EXPIRY_JIFFIES	msecs_to_jiffies(30000)
+
 /* Offsets into PTP packet for identification.  These offsets are from the
  * start of the IP header, not the MAC header.  Note that neither PTP V1 nor
  * PTP V2 permit the use of IPV4 options.
@@ -218,6 +221,7 @@  struct efx_ptp_timeset {
  * @ether_type: Network protocol of the filter (ETHER_P_IP / ETHER_P_IPV6)
  * @loc_port: UDP port of the filter (PTP_EVENT_PORT / PTP_GENERAL_PORT)
  * @loc_host: IPv4/v6 address of the filter
+ * @expiry: time when the filter expires, in jiffies
  * @handle: Handle ID for the MCDI filters table
  */
 struct efx_ptp_rxfilter {
@@ -225,6 +229,7 @@  struct efx_ptp_rxfilter {
 	__be16 ether_type;
 	__be16 loc_port;
 	__be32 loc_host[4];
+	unsigned long expiry;
 	int handle;
 };
 
@@ -1318,8 +1323,8 @@  static inline void efx_ptp_process_rx(struct efx_nic *efx, struct sk_buff *skb)
 	local_bh_enable();
 }
 
-static bool efx_ptp_filter_exists(struct list_head *ptp_list,
-				  struct efx_filter_spec *spec)
+static struct efx_ptp_rxfilter *
+efx_ptp_find_filter(struct list_head *ptp_list, struct efx_filter_spec *spec)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 
@@ -1327,10 +1332,19 @@  static bool efx_ptp_filter_exists(struct list_head *ptp_list,
 		if (rxfilter->ether_type == spec->ether_type &&
 		    rxfilter->loc_port == spec->loc_port &&
 		    !memcmp(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host)))
-			return true;
+			return rxfilter;
 	}
 
-	return false;
+	return NULL;
+}
+
+static void efx_ptp_remove_one_filter(struct efx_nic *efx,
+				      struct efx_ptp_rxfilter *rxfilter)
+{
+	efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
+				  rxfilter->handle);
+	list_del(&rxfilter->list);
+	kfree(rxfilter);
 }
 
 static void efx_ptp_remove_filters(struct efx_nic *efx,
@@ -1339,10 +1353,7 @@  static void efx_ptp_remove_filters(struct efx_nic *efx,
 	struct efx_ptp_rxfilter *rxfilter, *tmp;
 
 	list_for_each_entry_safe(rxfilter, tmp, ptp_list, list) {
-		efx_filter_remove_id_safe(efx, EFX_FILTER_PRI_REQUIRED,
-					  rxfilter->handle);
-		list_del(&rxfilter->list);
-		kfree(rxfilter);
+		efx_ptp_remove_one_filter(efx, rxfilter);
 	}
 }
 
@@ -1358,13 +1369,17 @@  static void efx_ptp_init_filter(struct efx_nic *efx,
 
 static int efx_ptp_insert_filter(struct efx_nic *efx,
 				 struct list_head *ptp_list,
-				 struct efx_filter_spec *spec)
+				 struct efx_filter_spec *spec,
+				 unsigned long expiry)
 {
 	struct efx_ptp_rxfilter *rxfilter;
 	int rc;
 
-	if (efx_ptp_filter_exists(ptp_list, spec))
+	rxfilter = efx_ptp_find_filter(ptp_list, spec);
+	if (rxfilter) {
+		rxfilter->expiry = expiry;
 		return 0;
+	}
 
 	rxfilter = kzalloc(sizeof(*rxfilter), GFP_KERNEL);
 	if (!rxfilter)
@@ -1378,6 +1393,7 @@  static int efx_ptp_insert_filter(struct efx_nic *efx,
 	rxfilter->ether_type = spec->ether_type;
 	rxfilter->loc_port = spec->loc_port;
 	memcpy(rxfilter->loc_host, spec->loc_host, sizeof(spec->loc_host));
+	rxfilter->expiry = expiry;
 	list_add(&rxfilter->list, ptp_list);
 
 	return 0;
@@ -1389,28 +1405,31 @@  static int efx_ptp_insert_filter(struct efx_nic *efx,
 
 static int efx_ptp_insert_ipv4_filter(struct efx_nic *efx,
 				      struct list_head *ptp_list,
-				      __be32 addr, u16 port)
+				      __be32 addr, u16 port,
+				      unsigned long expiry)
 {
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
 	efx_filter_set_ipv4_local(&spec, IPPROTO_UDP, addr, htons(port));
-	return efx_ptp_insert_filter(efx, ptp_list, &spec);
+	return efx_ptp_insert_filter(efx, ptp_list, &spec, expiry);
 }
 
 static int efx_ptp_insert_ipv6_filter(struct efx_nic *efx,
 				      struct list_head *ptp_list,
-				      struct in6_addr *addr, u16 port)
+				      struct in6_addr *addr, u16 port,
+				      unsigned long expiry)
 {
 	struct efx_filter_spec spec;
 
 	efx_ptp_init_filter(efx, &spec);
 	efx_filter_set_ipv6_local(&spec, IPPROTO_UDP, addr, htons(port));
-	return efx_ptp_insert_filter(efx, ptp_list, &spec);
+	return efx_ptp_insert_filter(efx, ptp_list, &spec, expiry);
 }
 
 static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 {
+	struct efx_ptp_data *ptp = efx->ptp_data;
 	const u8 addr[ETH_ALEN] = PTP_ADDR_ETHER;
 	struct efx_filter_spec spec;
 
@@ -1418,7 +1437,7 @@  static int efx_ptp_insert_eth_multicast_filter(struct efx_nic *efx)
 	efx_filter_set_eth_local(&spec, EFX_FILTER_VID_UNSPEC, addr);
 	spec.match_flags |= EFX_FILTER_MATCH_ETHER_TYPE;
 	spec.ether_type = htons(ETH_P_1588);
-	return efx_ptp_insert_filter(efx, &efx->ptp_data->rxfilters_mcast, &spec);
+	return efx_ptp_insert_filter(efx, &ptp->rxfilters_mcast, &spec, 0);
 }
 
 static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
@@ -1433,12 +1452,14 @@  static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 	 * that there is no packet re-ordering.
 	 */
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
-					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT);
+					htonl(PTP_ADDR_IPV4), PTP_EVENT_PORT,
+					0);
 	if (rc < 0)
 		goto fail;
 
 	rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_mcast,
-					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT);
+					htonl(PTP_ADDR_IPV4), PTP_GENERAL_PORT,
+					0);
 	if (rc < 0)
 		goto fail;
 
@@ -1449,12 +1470,12 @@  static int efx_ptp_insert_multicast_filters(struct efx_nic *efx)
 		struct in6_addr ipv6_addr = {{PTP_ADDR_IPV6}};
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
-						&ipv6_addr, PTP_EVENT_PORT);
+						&ipv6_addr, PTP_EVENT_PORT, 0);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_mcast,
-						&ipv6_addr, PTP_GENERAL_PORT);
+						&ipv6_addr, PTP_GENERAL_PORT, 0);
 		if (rc < 0)
 			goto fail;
 
@@ -1490,21 +1511,24 @@  static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 					 struct sk_buff *skb)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
+	unsigned long expiry;
 	int rc;
 
 	if (!efx_ptp_valid_unicast_event_pkt(skb))
 		return -EINVAL;
 
+	expiry = jiffies + UCAST_FILTER_EXPIRY_JIFFIES;
+
 	if (skb->protocol == htons(ETH_P_IP)) {
 		__be32 addr = ip_hdr(skb)->saddr;
 
 		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_EVENT_PORT);
+						addr, PTP_EVENT_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv4_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
+						addr, PTP_GENERAL_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 	} else if (efx_ptp_use_mac_tx_timestamps(efx)) {
@@ -1512,12 +1536,12 @@  static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 		struct in6_addr *addr = &ipv6_hdr(skb)->saddr;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_EVENT_PORT);
+						addr, PTP_EVENT_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 
 		rc = efx_ptp_insert_ipv6_filter(efx, &ptp->rxfilters_ucast,
-						addr, PTP_GENERAL_PORT);
+						addr, PTP_GENERAL_PORT, expiry);
 		if (rc < 0)
 			goto fail;
 	} else {
@@ -1531,6 +1555,17 @@  static int efx_ptp_insert_unicast_filter(struct efx_nic *efx,
 	return rc;
 }
 
+static void efx_ptp_drop_expired_unicast_filters(struct efx_nic *efx)
+{
+	struct efx_ptp_data *ptp = efx->ptp_data;
+	struct efx_ptp_rxfilter *rxfilter, *tmp;
+
+	list_for_each_entry_safe(rxfilter, tmp, &ptp->rxfilters_ucast, list) {
+		if (time_is_before_jiffies(rxfilter->expiry))
+			efx_ptp_remove_one_filter(efx, rxfilter);
+	}
+}
+
 static int efx_ptp_start(struct efx_nic *efx)
 {
 	struct efx_ptp_data *ptp = efx->ptp_data;
@@ -1631,6 +1666,8 @@  static void efx_ptp_worker(struct work_struct *work)
 
 	while ((skb = __skb_dequeue(&tempq)))
 		efx_ptp_process_rx(efx, skb);
+
+	efx_ptp_drop_expired_unicast_filters(efx);
 }
 
 static const struct ptp_clock_info efx_phc_clock_info = {