Re: [vpp-dev] Please include Fixes: tag for regression fix

[Mohsin Kazmi via lists.fd.io]

We can also add fixline to git to get the formatted output i.e.


vpp$ git fixline 3effb4e6

Fixes: 3effb4e63068 ("memif: integrate with new tx infra")

One can add the following to .gitconfig file: fixline = log -1 --abbrev=12 
--format='Fixes: %h (\"%s\")'

-br
Mohsin

From:  on behalf of "Damjan Marion via lists.fd.io" 

Reply-To: "dmar...@me.com" 
Date: Tuesday, November 2, 2021 at 5:44 PM
To: "Steven Luong (sluong)" 
Cc: "vpp-dev@lists.fd.io" 
Subject: Re: [vpp-dev] Please include Fixes: tag for regression fix


We should probably extend checkstyle to reject patch if there is “Type: fix” 
and there is no “Fixes: *”.
We can have special case “Fixes: unknown” to willingly bypass this check….

—
Damjan



> On 02.11.2021., at 18:17, steven luong via lists.fd.io 
>  wrote:
>
> Folks,
>
> In case you don’t already know, there is a tag called Fixes in the commit 
> message which allows one to specify if the current patch fixes a regression. 
> See an example usage in https://gerrit.fd.io/r/c/vpp/+/34212
>
> When you commit a patch which fixes a known regression, please make use of 
> the Fixes tag to benefit every consumer.
>
> Steven
>
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20411): https://lists.fd.io/g/vpp-dev/message/20411
Mute This Topic: https://lists.fd.io/mt/86771694/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Query regarding packet validation checks in ip6_local

[Florin Coras]

Hi Pankaj, 

I’ll leave the hop-by-hop questions to others that are more familiar with the 
code. As for the length check being done only on the else branch, 
ip6_tcp_udp_icmp_validate_checksum needs to compute the length so I’m guessing 
the expectation is that the checksum validation will fail for bogus length 
packets. Are you hitting any issues with that code? 

Regards,
Florin

> On Nov 2, 2021, at 11:50 AM, pankajmalhotr...@gmail.com wrote:
> 
> Hi,
> In file vnet/ip/ip6_forward.c(VPP 21.01), function ip6_local_inline(), 
> the node's packet processing has the following check: 
>  
>   if (PREDICT_FALSE (need_csum))
> {
>   flags = ip6_tcp_udp_icmp_validate_checksum (vm, b[0]);
>   good_l4_csum = flags & VNET_BUFFER_F_L4_CHECKSUM_CORRECT;
>   error = IP6_ERROR_UNKNOWN_PROTOCOL;
> }
>   else
> {
>   if (ip6_tcp_udp_icmp_bad_length (vm, b[0]))
> error = IP6_ERROR_BAD_LENGTH;
> }
>  
> Kindly explain the reason behind the checksum and the length validation being 
> under the if-else check, making the length validation not applicable for the 
> case when checksum validation is applicable. 
>  
> Also, in the function ip6_tcp_udp_icmp_bad_length (refer the below snippet), 
> the function seems to assume that the hop-by-hop extension header shall 
> always be present. Is it that, IPv6 hop-by-hop extension header has to be 
> mandatorily present?
>  
> static_always_inline u8
> ip6_tcp_udp_icmp_bad_length (vlib_main_t * vm, vlib_buffer_t * p0)
> {
>  
>   u16 payload_length_host_byte_order;
>   u32 n_this_buffer, n_bytes_left;
>   ip6_header_t *ip0 = vlib_buffer_get_current (p0);
>   u32 headers_size = sizeof (ip0[0]);
>   u8 *data_this_buffer;
>  
>   data_this_buffer = (u8 *) (ip0 + 1);
>  
>   ip6_hop_by_hop_ext_t *ext_hdr = (ip6_hop_by_hop_ext_t *) data_this_buffer;
>  
>   /* validate really icmp6 next */
>  
>   if (!(ext_hdr->next_hdr == IP_PROTOCOL_ICMP6)
>   || (ext_hdr->next_hdr == IP_PROTOCOL_UDP))
> return 0;
>   ::
>   ::
> 
> 
> Thanks,
> Pankaj Malhotra 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20410): https://lists.fd.io/g/vpp-dev/message/20410
Mute This Topic: https://lists.fd.io/mt/86774091/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[vpp-dev] Query regarding packet validation checks in ip6_local

[pankajmalhotra83]

Hi,
In file vnet/ip/ip6_forward.c(VPP 21.01), function ip6_local_inline(), the 
node's packet processing has the following check:

if (PREDICT_FALSE (need_csum))
{
flags = ip6_tcp_udp_icmp_validate_checksum (vm, b[0]);
good_l4_csum = flags & VNET_BUFFER_F_L4_CHECKSUM_CORRECT;
error = IP6_ERROR_UNKNOWN_PROTOCOL;
}
else
{
if (ip6_tcp_udp_icmp_bad_length (vm, b[0]))
error = IP6_ERROR_BAD_LENGTH;
}

Kindly explain the reason behind the checksum and the length validation being 
under the if-else check, making the length validation not applicable for the 
case when checksum validation is applicable.

Also, in the function ip6_tcp_udp_icmp_bad_length (refer the below snippet), 
the function seems to assume that the hop-by-hop extension header shall always 
be present. Is it that, IPv6 hop-by-hop extension header has to be mandatorily 
present?

static_always_inline u8
ip6_tcp_udp_icmp_bad_length (vlib_main_t * vm, vlib_buffer_t * p0)
{

u16 payload_length_host_byte_order;
u32 n_this_buffer, n_bytes_left;
ip6_header_t *ip0 = vlib_buffer_get_current (p0);
u32 headers_size = sizeof (ip0[0]);
u8 *data_this_buffer;

data_this_buffer = (u8 *) (ip0 + 1);

ip6_hop_by_hop_ext_t *ext_hdr = (ip6_hop_by_hop_ext_t *) data_this_buffer;

/* validate really icmp6 next */

if (!(ext_hdr->next_hdr == IP_PROTOCOL_ICMP6)
|| (ext_hdr->next_hdr == IP_PROTOCOL_UDP))
return 0;
::
::

Thanks,
Pankaj Malhotra

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20409): https://lists.fd.io/g/vpp-dev/message/20409
Mute This Topic: https://lists.fd.io/mt/86774091/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] det44 plugin

[Filip Varga via lists.fd.io]

Hi Ben,

Thank you for pointing out the issue. Indeed it looks like the node runs just 
once. I will provide a patch shortly.

Best regards,
Filip Varga


-Original Message-
From: vpp-dev@lists.fd.io  On Behalf Of Ben McKeegan
Sent: Monday, November 1, 2021 7:24 PM
To: vpp-dev@lists.fd.io
Subject: [vpp-dev] det44 plugin

Hello,

I am fairly new to VPP so please bear with me.  I am trying to use the
det44 NAT plugin on 21.06 but I am experiencing some difficulties with 
running out of ports.   It would appear that my det44 sessions are never 
removed despite passing the expire time.   For example, I have the 
following setting:

show det44 timeout
udp timeout: 300sec
tcp established timeout: 7440sec
tcp transitory timeout: 240sec
icmp timeout: 60sec

However, if I generate a series of ICMP pings from my test host and then run 
'show det44 sessions' I have a session listed for every individual ping packet, 
as expected, but these remain long after the 60 second timeout configured.  For 
example on my last test I sent a flood of 100 pings which generated 100 
sessions in the lists, all "state: icmp-active 
expire" with expiry times ranging from 171 to 173.   I have just sent 
another 100 pings and now have another 100 sessions with expiry times ranging 
from 2647 to 2650, and the original 100 sessions are still there still with 
expiry times from 171 to 173 so these have not been refreshed or expired.

I have taken a look at the source code of the plugin and I can that
det44_create_expire_walk_process() is called from det44_plugin_enable(). 
  This function appears to start a new vlib process with the 'main loop' 
  function det44_expire_walk_fn().

According to the documentation here
https://docs.fd.io/vpp/21.06/dd/d64/vlib__process__doc_8h.html I understand 
these despatch functions should be implemented as a while (1) {} loop that 
never ends.  However, my reading of the
det44_expire_walk_fn() function code is that it will only perform a single walk 
of the det44 data structures before returning to its caller.

Is this a bug in det44_expire_walk_fn(), is the documentation wrong or 
am I misreading it?   My hypothesis is that det44_expire_walk_fn() runs 
just once, when the plugin is first enabled (and the session table is already 
empty), and does not get run again thereafter.  Therefore, the sessions never 
get expired.


Regards,
Ben.





-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20408): https://lists.fd.io/g/vpp-dev/message/20408
Mute This Topic: https://lists.fd.io/mt/86748366/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Please include Fixes: tag for regression fix

[Damjan Marion via lists.fd.io]

We should probably extend checkstyle to reject patch if there is “Type: fix” 
and there is no “Fixes: *”.
We can have special case “Fixes: unknown” to willingly bypass this check….

— 
Damjan



> On 02.11.2021., at 18:17, steven luong via lists.fd.io 
>  wrote:
> 
> Folks,
>  
> In case you don’t already know, there is a tag called Fixes in the commit 
> message which allows one to specify if the current patch fixes a regression. 
> See an example usage in https://gerrit.fd.io/r/c/vpp/+/34212
>  
> When you commit a patch which fixes a known regression, please make use of 
> the Fixes tag to benefit every consumer.
>  
> Steven
>  
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20407): https://lists.fd.io/g/vpp-dev/message/20407
Mute This Topic: https://lists.fd.io/mt/86771694/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[vpp-dev] Please include Fixes: tag for regression fix

[steven luong via lists.fd.io]

Folks,

In case you don’t already know, there is a tag called Fixes in the commit 
message which allows one to specify if the current patch fixes a regression. 
See an example usage in https://gerrit.fd.io/r/c/vpp/+/34212

When you commit a patch which fixes a known regression, please make use of the 
Fixes tag to benefit every consumer.

Steven


-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20406): https://lists.fd.io/g/vpp-dev/message/20406
Mute This Topic: https://lists.fd.io/mt/86771694/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] VPP binary API to make interface state up

[Mohsin Kazmi via lists.fd.io]

You are setting the “mp->flags = 0;” as (IF_STATUS_API_FLAG_LINK_UP & 
IF_STATUS_API_FLAG_ADMIN_UP == 0)

To ‘UP’ the interface:
u32 flags = IF_STATUS_API_FLAG_ADMIN_UP;

 mp->flags = clib_host_to_net_u32 (flags);

To ‘DOWN’ the interface:
  u32 flags = 0;

  mp->flags = clib_host_to_net_u32 (flags);


-br
Mohsin
From:  on behalf of Subrata Nath 
Date: Tuesday, November 2, 2021 at 4:09 PM
To: "vpp-dev@lists.fd.io" 
Subject: [vpp-dev] VPP binary API to make interface state up

HI,

I am trying to implement the following vppctl command through C API. vpp 
version 20.09.

vppctl set int ip addr host-eth3 10.22.6.20/24
vppctl set int state host-eth3 up

First command i.e. to plumb the IP works fine through the program attached but 
to make the interface up, i am using vl_api_sw_interface_set_flags_t message.

 vl_api_sw_interface_set_flags_t *mp;

mp = vl_msg_api_alloc (sizeof (*mp));
clib_memset (mp, 0, sizeof (*mp));

mp->_vl_msg_id = clib_host_to_net_u16  (VL_API_SW_INTERFACE_SET_FLAGS);
mp->client_index = api_client_index;
mp->context = vpp_client_context;
mp->sw_if_index = clib_host_to_net_u32  (ifindex);
//mp->flags = IF_STATUS_API_FLAG_ADMIN_UP;
mp->flags = IF_STATUS_API_FLAG_LINK_UP & IF_STATUS_API_FLAG_ADMIN_UP;
vl_msg_api_send_shmem (api_vl_input_queue, (u8 *) & mp);

reply handler says retval is success but i don't see interface state is up 
through vppctl. vppctl command shows IP is plumbed fine. Can anyone suggest 
what's the issue?

Also, In this version, vl_api_sw_interface_set_flags_t  flag parameter is a 
enum with IF_STATUS_API_FLAG_ADMIN_UP , IF_STATUS_API_FLAG_LINK_UP. So I am not 
sure how to make the interface state down.

Thanks in advance.

Regards,
Subrata

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20405): https://lists.fd.io/g/vpp-dev/message/20405
Mute This Topic: https://lists.fd.io/mt/86769737/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[vpp-dev] VPP binary API to make interface state up

[Subrata Nath]

HI,

I am trying to implement the following vppctl command through C API. vpp
version 20.09.


*vppctl set int ip addr host-eth3 10.22.6.20/24
vppctl set int state host-eth3 up*

First command i.e. to plumb the IP works fine through the program attached
but to make the interface up, i am using vl_api_sw_interface_set_flags_t
message.

 vl_api_sw_interface_set_flags_t *mp;

mp = vl_msg_api_alloc (sizeof (*mp));
clib_memset (mp, 0, sizeof (*mp));

mp->_vl_msg_id = clib_host_to_net_u16
 (VL_API_SW_INTERFACE_SET_FLAGS);
mp->client_index = api_client_index;
mp->context = vpp_client_context;
mp->sw_if_index = clib_host_to_net_u32  (ifindex);
//mp->flags = IF_STATUS_API_FLAG_ADMIN_UP;
mp->flags = IF_STATUS_API_FLAG_LINK_UP &
IF_STATUS_API_FLAG_ADMIN_UP;
vl_msg_api_send_shmem (api_vl_input_queue, (u8 *) & mp);

reply handler says retval is success but i don't see interface state is up
through vppctl. vppctl command shows IP is plumbed fine. Can anyone suggest
what's the issue?

Also, In this version, vl_api_sw_interface_set_flags_t  flag parameter is a
enum with IF_STATUS_API_FLAG_ADMIN_UP , IF_STATUS_API_FLAG_LINK_UP. So I am
not sure how to make the interface state down.

Thanks in advance.

Regards,
Subrata


VPP_binary_API_client.c
Description: Binary data

-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#20404): https://lists.fd.io/g/vpp-dev/message/20404
Mute This Topic: https://lists.fd.io/mt/86769737/21656
Group Owner: vpp-dev+ow...@lists.fd.io
Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [vpp-dev] Bihash is considered thread-safe but probably shouldn't

[Dave Barach]

Dear Nick,

 

As the code comment suggests, we tiptoe right up to the line to extract 
performance. Have you tried e.g. ISOLCPUS, thread priority, or some other 
expedients to make the required assumptions true?

 

It’s easy enough to change the code in various ways so this use-case cannot 
backfire. High on the list: always make a working copy of the bucket, vs. 
update in place. Won’t help write performance, but it’s likely to make the pain 
go away.

 

Bucket-level reader-locks would involve adding Avogadro’s number of atomic ops 
to the predominant case. I’m pretty sure that’s a non-starter.

 

FWIW... Dave

 

 

From: vpp-dev@lists.fd.io  On Behalf Of Nick Zavaritsky
Sent: Monday, November 1, 2021 12:12 PM
To: vpp-dev@lists.fd.io
Subject: [vpp-dev] Bihash is considered thread-safe but probably shouldn't

 

Hello bihash experts!

There's an old thread claiming that bihash lookup can produce a value=-1 under 
intense add/delete concurrent activity: 
https://lists.fd.io/g/vpp-dev/message/15606

We had a seemingly related crash recently when a lookup in snat_main.flow_hash 
yielded a value=-1 which was subsequently used as a destination thread index to 
offload to. This crash prompted me to study bihash more closely.

The rest of the message is structured as follows:
  1. Presenting reasons why I believe that bihash is not thread-safe.
  2. Proposing a fix.

1 Bihash is probably not thread-safe

The number of buckets in a hash table never changes. Every bucket has a lock 
bit. Updates happen via clib_bihash_add_del_inline_with_hash. The function 
grabs the bucket lock early on and performs update while holding the lock. 
Obviously this is safe, let's focus on readers.

Lookups happen via clib_bihash_search_inline_with_hash / 
clib_bihash_search_inline_2_with_hash. The function locates the bucket and 
waits until the lock bit is cleared.

  b = BV (clib_bihash_get_bucket) (h, hash);

 

  if (PREDICT_FALSE (BV (clib_bihash_bucket_is_empty) (b)))

return -1;

 

  if (PREDICT_FALSE (b->lock))

{

  volatile BVT (clib_bihash_bucket) * bv = b;

  while (bv->lock)

CLIB_PAUSE ();

}


>From this point on the function examines the data structure without ever 
>bothering to check the lock again. Nothing prevents an updater from changing 
>the data the reader is currently looking at, or even deallocating it right 
>away. The only way it could work is if we could make assumptions about 
>relative performance of lookup and update operations. Checking the lock early 
>in lookup ensures that there's no update currently in progress. If lookup is 
>faster than update, then no future updater will manage to progress to the 
>point where the memory is written BEFORE the lookup was complete. Indeed, we 
>have the following comment in clib_bihash_add_del_inline_with_hash:

  /*

   * Because reader threads are looking at live data,

   * we have to be extra careful. Readers do NOT hold the

   * bucket lock. We need to be SLOWER than a search, past the

   * point where readers CHECK the bucket lock.

   */

Unfortunately, the assumption doesn't hold. Any thread could get preempted at 
arbitrary time. Even if we rule out preemption, there are microarchitectural 
quirks (e.g. caches, branch misprediction) that could slow down lookup to the 
point that memory read and update will overlap. 


The core of lookup is the following loop. Please note that checking a key and 
fetching the value is not atomic, hence if we are preempted in-between the 
result could be bogus.

  for (i = 0; i < limit; i++)

{

  if (BV (clib_bihash_key_compare) (v->kvp[i].key, key_result->key))

{

  *key_result = v->kvp[i];

  return 0;

}

}


Different ways the key-value pair could get updated:

(1) Add using a previously empty slot:

  clib_memcpy_fast (&(v->kvp[i].value),

_v->value, sizeof (add_v->value));

  CLIB_MEMORY_STORE_BARRIER (); /* Make sure the value has 
settled */

  clib_memcpy_fast (&(v->kvp[i]), _v->key,

sizeof (add_v->key));


The key update is not atomic, reader could observe a key updated half-way.


(2) Add that recycles a stale slot:

  clib_memcpy_fast (&(v->kvp[i]), add_v, sizeof (*add_v));


Yet again not atomic. A reader could witness (old_k, new_v) or (new_k, old_v) 
or even an arbitrary interleaving of chunks from old and new keys.

(3) Deleting an entry:

clib_memset_u8 (&(v->kvp[i]), 0xff, sizeof (*(add_v)));

Not atomic.


2 A fix

It's worth noting that bihash never crashes. It does yield bogus results 
occasionally, though. While -1 is easy to check for, the analysis shows that 
other bogus results are possible. In particular:

  1. Value updated half-way, possible with bihash_8_16.
  2. Observing a key that never existed due to a key partial update. The 
probability is low since the hash should