[dhcpmon] Fix dhcpmon socket filter and tx count issue #13065

kellyyeh · 2022-12-15T06:30:54Z

Why I did it

Fix issue caused by dualtor support PR [dhcpmon] Open different socket for dual tor to enable interface filtering #11201
This issue exists for version 20201231.73 - 20201231.86 on single and dual tor
- From the above PR, dhcpmon only listens for packets on active ToR and not standby ToR, but dhcp packet count is more than 3 times the count. The inbound filter listening on all interfaces causes it to pick up more than one copy of the packet, including Ethxx, lo, Bridge. Tx counter does not increase count accordingly due to filter set to inbound.
  Below is an example of dhcpmon counter without fix:
  -dhcp_relay#dhcpmon[58]: DHCPv4 [ Agg-Vlan2000-Snapshot rx/tx] Discover: 4/ 0, Offer: 4/ 0, Request: 4/ 0, ACK: 4/ 0
  In opposed to dhcpmon with correct counter:
  -dhcp_relay#dhcpmon[58]: DHCPv4 [ Agg-Vlan2000-Snapshot rx/tx] Discover: 1/ 4, Offer: 1/ 4, Request: 1/ 4, ACK: 1/ 4
Improve code

How I did it

On single ToR, packets received count was duplicated due to socket filter set to "inbound"
Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets
Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27
Exit when socket fails to initialize to allow dhcp_relay docker to restart

How to verify it

Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct

Correct number of rx increases
Tx does not increase when ToR is on standby

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Improve code

How I did it

On single ToR, packets received count was duplicated due to socket filter set to "inbound"
Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets
Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27
Exit when socket fails to initialize to allow dhcp_relay docker to restart

How to verify it

Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct

Correct number of tx increases
Tx does not increase when ToR is on standby

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

jcaiMR · 2022-12-15T16:40:26Z

src/dhcpmon/src/dhcp_device.cpp

+        while ((offset < (dhcp_option_sz + 1)) && dhcp_option[offset] != 255) {
+            switch (dhcp_option[offset])
+            {
+            case 53:


Better to use enum for option53, like:
enum {
OptionDHCPMessageType = 53,
}
If we only care about option53, maybe we don;t need switch case here, just use if and remove temp variable stop_dhcp_processing.

jcaiMR · 2022-12-15T17:02:00Z

I think current diff still has duplicate input packets issue,
Each time we call dhcp_devman_add_intf( ) will have a rx socket monitor on all interfaces, kernel will duplicate packets to all matched sockets.

Normally, for socket listen on all interfaces, we just need one instance and give this socket a big enough rcvd buffer size.
For TX socket as it bind to particular interface, so each interface has a socket would be fine.

And if we use one socket for all input interface binding, better to use batch process for better performance, that is one read ready event will trigger a batch of recvfrom operations.

kellyyeh · 2022-12-22T00:42:29Z

Each dhcpmon process will only monitor one vlan

sonic-buildimage/dockers/docker-dhcp-relay/dhcp-relay.monitors.j2

Line 44 in 3740f1e

command=/usr/sbin/dhcpmon -id {{ vlan_name }}

jcaiMR · 2023-01-04T12:22:48Z

src/dhcpmon/src/dhcp_device.cpp

@@ -252,62 +308,28 @@ static void read_callback_dual_tor(int fd, short event, void *arg)
    while ((event == EV_READ) &&


Same as above.

jcaiMR · 2023-01-04T12:23:07Z

src/dhcpmon/src/dhcp_device.cpp

 *
 * @param fd            socket to read from
 * @param event         libevent triggered event
 * @param arg           user provided argument for callback (interface context)
 *
 * @return none
 */
-static void read_callback(int fd, short event, void *arg)
+static void read_tx_callback(int fd, short event, void *arg)
 {
    dhcp_device_context_t *context = (dhcp_device_context_t*) arg;
    ssize_t buffer_sz;

    while ((event == EV_READ) &&


Here "event == EV_READ" will always true as we register this handler for EV_READ, we don't need to check it again.

event could be EV_PERIST or EV_READ

To be accurate my first comments should be event & EV_READ will always true. But in logic, EV_PERIST controls the event is persistent, it makes you don't need to use event_add() again after callback. So when callback is called it must be a READ event, as you don't register read_tx_callback to any WRITE/DELETE events.
So we don't need this check here, unless sometime you defined a unique function for all READ/WRITE/DELETE events, then in that function we may need this event value check.

Thanks I see. Removed event check

jcaiMR · 2023-01-04T12:24:58Z

src/dhcpmon/src/dhcp_device.cpp

+        tx_addr.sll_ifindex = if_nametoindex(intf);
+        tx_addr.sll_family = AF_PACKET;
+        tx_addr.sll_protocol = htons(ETH_P_ALL);
+        if (bind(context->tx_sock, (struct sockaddr *) &tx_addr, sizeof(tx_addr))) {
            syslog(LOG_ALERT, "bind: failed to bind to interface '%s' with '%s'\n", intf, strerror(errno));
            break;


If we failed to bind socket, we may just call exit() to let container restart.

jcaiMR · 2023-01-04T13:48:25Z

src/dhcpmon/src/dhcp_device.cpp

-        if (context->sock < 0) {
+        context->rx_sock = socket(AF_PACKET, SOCK_RAW | SOCK_NONBLOCK, htons(ETH_P_ALL));
+        context->tx_sock = socket(AF_PACKET, SOCK_RAW | SOCK_NONBLOCK, htons(ETH_P_ALL));
+        if (context->rx_sock < 0 || context->tx_sock < 0) {
            syslog(LOG_ALERT, "socket: failed to open socket with '%s'\n", strerror(errno));
            break;


if socket failed to create, we can just exit program.

jcaiMR · 2023-01-04T13:50:15Z

src/dhcpmon/src/dhcp_device.cpp

@@ -666,21 +700,27 @@ int dhcp_device_start_capture(dhcp_device_context_t *context,
        }
        context->snaplen = snaplen;

-        if (setsockopt(context->sock, SOL_SOCKET, SO_ATTACH_FILTER, &dhcp_sock_bfp, sizeof(dhcp_sock_bfp)) != 0) {
+        if (setsockopt(context->rx_sock, SOL_SOCKET, SO_ATTACH_FILTER, &dhcp_inbound_sock_bfp, sizeof(dhcp_inbound_sock_bfp)) != 0) {
            syslog(LOG_ALERT, "setsockopt: failed to attach filter with '%s'\n", strerror(errno));


Same like above, maybe we just exit program if any error happened in this function.

Updated for the function

qiluo-msft · 2023-01-11T23:30:50Z

What issue is fixed by this PR? could you explain in PR description?

yxieca · 2023-01-12T22:45:36Z

@kellyyeh is there a particular reason you didn't request to backport to 202211 branch?

mssonicbld · 2023-01-12T22:45:37Z

@kellyyeh PR conflicts with 202205 branch

yxieca · 2023-01-12T22:46:14Z

@kellyyeh please create PR for 202205 branch for identify missing dependencies

kellyyeh · 2023-01-12T22:50:55Z

@yxieca PR #11201 is not cherry-picked to 202211 yet. Just added label for both PRs

qiluo-msft · 2023-01-12T23:34:25Z

This commit could not be cleanly cherry-picked to 202012. Please submit another PR.

yxieca · 2023-01-19T17:25:51Z

@kellyyeh this change cannot be cherry-picked cleanly to 20205 branch. can you raise separate PR?

Why I did it Fix issue caused by dualtor support PR [dhcpmon] Open different socket for dual tor to enable interface filtering sonic-net#11201 Improve code How I did it On single ToR, packets received count was duplicated due to socket filter set to "inbound" Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27 Exit when socket fails to initialize to allow dhcp_relay docker to restart How to verify it Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct Correct number of tx increases Tx does not increase when ToR is on standby

Why I did it Fix issue caused by dualtor support PR [dhcpmon] Open different socket for dual tor to enable interface filtering #11201 Improve code How I did it On single ToR, packets received count was duplicated due to socket filter set to "inbound" Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27 Exit when socket fails to initialize to allow dhcp_relay docker to restart How to verify it Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct Correct number of tx increases Tx does not increase when ToR is on standby

liuh-80 · 2023-02-02T01:54:57Z

202012 branch PR merged: #13441

Why I did it Fix issue caused by dualtor support PR [dhcpmon] Open different socket for dual tor to enable interface filtering sonic-net#11201 Improve code How I did it On single ToR, packets received count was duplicated due to socket filter set to "inbound" Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27 Exit when socket fails to initialize to allow dhcp_relay docker to restart How to verify it Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct Correct number of tx increases Tx does not increase when ToR is on standby

mssonicbld · 2023-02-03T15:45:27Z

Cherry-pick PR to 202211: #13638

Why I did it Fix issue caused by dualtor support PR [dhcpmon] Open different socket for dual tor to enable interface filtering #11201 Improve code How I did it On single ToR, packets received count was duplicated due to socket filter set to "inbound" Tx count not increasing due to filter set to "inbound". Added an outbound socket to count tx packets Added vlan member interface mapping for Ethernet interface to vlan interface lookup in reference to PR Fix multiple vlan issue sonic-dhcp-relay#27 Exit when socket fails to initialize to allow dhcp_relay docker to restart How to verify it Tested on vstestbed single tor and dual tor, sent packets and verify printed out dhcpmon rx and tx counters is correct Correct number of tx increases Tx does not increase when ToR is on standby

Fix dhcpmon socket filter issue

8d955b2

jcaiMR reviewed Dec 15, 2022

View reviewed changes

Address PR comment

8bfb0d1

kellyyeh marked this pull request as ready for review December 22, 2022 01:30

kellyyeh requested a review from jcaiMR January 3, 2023 17:48

jcaiMR reviewed Jan 4, 2023

View reviewed changes

Exit program when socket fails to initialize

fecc41d

kellyyeh requested a review from jcaiMR January 5, 2023 02:58

delete ev_read check

3f69444

jcaiMR approved these changes Jan 6, 2023

View reviewed changes

kellyyeh requested a review from yxieca January 6, 2023 22:19

kellyyeh added Request for 202012 Branch Request for 202205 Branch labels Jan 6, 2023

yxieca approved these changes Jan 6, 2023

View reviewed changes

yxieca merged commit 2c410b4 into sonic-net:master Jan 6, 2023

kellyyeh deleted the dhcpmon-fix branch January 6, 2023 22:43

qiluo-msft added Request for 202012 Branch and removed Request for 202012 Branch labels Jan 11, 2023

yxieca added the Approved for 202205 Branch label Jan 12, 2023

mssonicbld added the Cherry Pick Conflict_202205 label Jan 12, 2023

kellyyeh added the Request for 202211 Branch label Jan 12, 2023

qiluo-msft added the Cherry Pick Conflict_202012 label Jan 12, 2023

yxieca added Cherry Pick Conflict_202205 and removed Cherry Pick Conflict_202205 labels Jan 19, 2023

kellyyeh mentioned this pull request Jan 19, 2023

[202205][dhcpmon] Fix dhcpmon socket filter and tx count issue (#13065) #13439

Merged

8 tasks

kellyyeh mentioned this pull request Jan 19, 2023

[202012][dhcpmon] Fix dhcpmon socket filter and tx count issue (#13065) #13441

Merged

8 tasks

liuh-80 added the Included in 202012 Branch label Feb 2, 2023

yxieca added the Included in 202205 Branch label Feb 2, 2023

StormLiangMS added the Approved for 202211 Branch label Feb 3, 2023

mssonicbld added the Created PR to 202211 Branch label Feb 3, 2023

mssonicbld mentioned this pull request Feb 3, 2023

[action] [PR:13065] [dhcpmon] Fix dhcpmon socket filter and tx count issue #13638

Merged

mssonicbld added Included in 202211 Branch and removed Approved for 202211 Branch Created PR to 202211 Branch labels Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dhcpmon] Fix dhcpmon socket filter and tx count issue #13065

[dhcpmon] Fix dhcpmon socket filter and tx count issue #13065

kellyyeh commented Dec 15, 2022 •

edited

Loading

jcaiMR Dec 15, 2022

kellyyeh Jan 5, 2023

jcaiMR commented Dec 15, 2022 •

edited

Loading

kellyyeh commented Dec 22, 2022

jcaiMR Jan 4, 2023 •

edited

Loading

jcaiMR Jan 4, 2023

kellyyeh Jan 5, 2023

jcaiMR Jan 5, 2023 •

edited

Loading

kellyyeh Jan 6, 2023

jcaiMR Jan 4, 2023

kellyyeh Jan 5, 2023

jcaiMR Jan 4, 2023

kellyyeh Jan 5, 2023

jcaiMR Jan 4, 2023

kellyyeh Jan 5, 2023

qiluo-msft commented Jan 11, 2023

yxieca commented Jan 12, 2023

mssonicbld commented Jan 12, 2023

yxieca commented Jan 12, 2023

kellyyeh commented Jan 12, 2023

qiluo-msft commented Jan 12, 2023

yxieca commented Jan 19, 2023

liuh-80 commented Feb 2, 2023

mssonicbld commented Feb 3, 2023

		@@ -252,62 +308,28 @@ static void read_callback_dual_tor(int fd, short event, void *arg)
		while ((event == EV_READ) &&

[dhcpmon] Fix dhcpmon socket filter and tx count issue #13065

[dhcpmon] Fix dhcpmon socket filter and tx count issue #13065

Conversation

kellyyeh commented Dec 15, 2022 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcaiMR commented Dec 15, 2022 • edited Loading

kellyyeh commented Dec 22, 2022

jcaiMR Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcaiMR Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Jan 11, 2023

yxieca commented Jan 12, 2023

mssonicbld commented Jan 12, 2023

yxieca commented Jan 12, 2023

kellyyeh commented Jan 12, 2023

qiluo-msft commented Jan 12, 2023

yxieca commented Jan 19, 2023

liuh-80 commented Feb 2, 2023

mssonicbld commented Feb 3, 2023

kellyyeh commented Dec 15, 2022 •

edited

Loading

jcaiMR commented Dec 15, 2022 •

edited

Loading

jcaiMR Jan 4, 2023 •

edited

Loading

jcaiMR Jan 5, 2023 •

edited

Loading